Hooks Amendment #4225
Hooks Amendment #4225
Conversation
There was a problem hiding this comment.
This is a huge and hugely complicated PR, so it will take a while. This is my first run through this code and it's causing my browser a lot of grief, given its size.
Expect follow-up reviews, but there are a couple of issues that need to be addressed already and I figured this was a good place to stop.
| enum hook_log_code : uint16_t | ||
| { | ||
| /* HookSet log-codes */ | ||
| AMENDMENT_DISABLED = 1, // attempt to HookSet when amendment is not yet enabled. |
There was a problem hiding this comment.
I would suggesting avoiding explicitly numbering these. Number the first as 1 and let the compiler do the tedious work; just leave a note to never remove entries from this enum.
There was a problem hiding this comment.
I agree with you, but these are used for external tools (such as the Hooks Builder) and being able to look up the number pre-populated in the source is very useful for those tools and their developers. It's also a strong reminder to developers not to insert new codes out of order which might change existing codes. (Although as you say a comment would work too.)
| } | ||
|
|
||
| enum hook_return_code : int64_t | ||
| { |
There was a problem hiding this comment.
Existing serializers don't have support for signed 64-bit integers, so you'll end up having to cast things. Why not change this to std::uint64_t and just treat it the way Microsoft does HRESULT? Reserve some of the high bits (e.g. the top four) to indicate different classes of errors. Anything that has one of those four bits set is an error.
There was a problem hiding this comment.
This enum is only used by the Hook API itself, so these are values returned by a host function to the running web assembly (analogous to the way a system call returns a value to a userspace program, where in this case rippled is the kernel and the hook is the userspace program). The result is never serialized to metadata or anywhere else, unless the hook developer serializes into hook state. Using int64_t allows for a very simple way for hook developers to test for an error, namely if (hook_api_func() < 0) //error logic here. Changing it to a uint64_t is perfectly possible, as you point out, but then hook developers would be writing if (hook_api_func() >> 63U) //error logic here this is possible but I think developing hooks is already difficult enough without making them jump through bit shifting hoops just to test for errors. Can revisit if you think important.
| @@ -456,6 +593,11 @@ Transactor::apply() | |||
| NotTEC | |||
| Transactor::checkSign(PreclaimContext const& ctx) | |||
| { | |||
| // hook emitted transactions do not have signatures | |||
| if (ctx.view.rules().enabled(featureHooks) && | |||
There was a problem hiding this comment.
This is a really scary bit of code here. We need to be 1000% sure that a transaction that passes this if can NEVER be generated from something other than a hook under ANY circumstances.
It might actually be nice to have a test here that verifies that the account has authorized the hook that issued the transaction to issue transactions on its behalf.
There was a problem hiding this comment.
Definitely agree that this is a dangerous and pivotal piece of code.
The isEmittedTxn function tests for the presence of the sfEmitDetails block in a transaction. Provided there's no way to put that validly into a non-emitted transaction then this condition will never pass. This is already blocked at the netcode in 5 different locations... however given how cheap it is to check, the more checks the better. (Ultimately it could actually be checked in TxQ. Unless it's added from the emitDir it should never have sfEmitDetails.)
It is possible to check if the Hook that emitted the txn was authorized by the emitting account, but there's an annoying corner case here: since the emitted txn is "from the past" i.e. created by a past execution of a hook, the current state of the ledger might not be the same as it was then. That is: the emitted txn was authorized when it was emitted but at the time the signature is being checked (at insertion into the next ledger, or the one after that) it might not be. And a very real case of this is when a Hook emits a txn that deletes itself. The "one and done" self deleting hook is actually a very useful construct, and not something we want to accidentally prevent.
We could just check the historic ledger. But if we do this then we need to ensure a validator resyncing has at least that much history before this code ever executes. (Even though it's only 5 ledgers, I'm not sure if that is necessarily the case at the moment?)
|
Thanks for the review, Nik. I will get on to these suggestions when back at work |
Thank me after I'm done reviewing. This was just the opening salvo. Enjoy your vacation 😃 |
|
@RichardAH - I think the issue might be that you used curly braces for the std::string constructor. Can you try using parentheses?
`std::string raddr((char*)(memory + read_ptr), read_len);`
greg
|
std::string raddr((char*)(memory + read_ptr), read_len);
auto const result = decodeBase58Token(raddr, TokenType::AccountID);
if (result.empty())
return INVALID_ARGUMENT;
WRITE_WASM_MEMORY_AND_RETURN(
write_ptr, write_len,
result.data(), 20,
memory, memory_length);Still doesn't work for me. Can you please test yourself also? It's technically possible my specific build environment is responsible for the outcome I see |
Hum, I apologize, I'm new at Ripple and I don't know how I would test this. I don't see a JTX test for this. I tried building your branch on windows, but there are errors: The first one is because the enum clashes with the windows macro defined in |
I've actually seen this conflict before (can't recall when). It doesn't occur on the compilers I use but I will change the name for you now. |
| #define NOT_IN_BOUNDS(ptr, len, memory_length)\ | ||
| ((static_cast<uint64_t>(ptr) > static_cast<uint64_t>(memory_length)) || \ | ||
| ((static_cast<uint64_t>(ptr) + static_cast<uint64_t>(len)) > static_cast<uint64_t>(memory_length))) |
There was a problem hiding this comment.
The first part of the || is unnecessary. The following is enough:
#define NOT_IN_BOUNDS(ptr, len, memory_length)\ ((static_cast<uint64_t>(ptr) + static_cast<uint64_t>(len)) > static_cast<uint64_t>(memory_length))
There was a problem hiding this comment.
👍Good point
From memory the lower bound check was actually (although not obviously) to help check for an overflow, but the inputs are always going to be i32 from web assembly and it's an incomplete overflow check anyway. Since these are very inexpensive checks (fastest possible CPU operations) I think we should go with a better defensive (not strictly necessary) overflow check instead of the first part of the || like so:
static_cast<uint64_t>(ptr) + static_cast<uint64_t>(len) < std::min(static_cast<uint64_t>(ptr), static_cast<uint64_t>(len))Happy to hear your thoughts
There was a problem hiding this comment.
Maybe I am missing something, but I think this is equivalent to a + b < min(a, b) which, considering a and b are positive, will never be true. If the original i32 numbers can be negative maybe we can do a better check?
There was a problem hiding this comment.
If for whatever reason at some point this bounds check macro is used with uint64_t's (keeping in mind macro arguments are not typed checked) then it's best to check for overflow here as a defensive programming principle. a + b can be less than min(a,b) if a+b overflows the uint64_t.
Here's an illustration:
#include <stdint.h>
#include <iostream>
#include <algorithm>
int main() {
auto overflowTest = [](uint64_t a, uint64_t b)
{
uint64_t c = a + b;
std::cout <<
"a=" << a << ", "
"b=" << b << ". "
"a+b=" << (a+b) << ". "
"a + b < min(a,b) ? " << (a + b < std::min(a,b) ? "yes": "no") << "\n";
};
// without an overflow event
overflowTest(0xFFFFFFFFFFFFFFFDULL, 2);
// with an overflow event
overflowTest(0xFFFFFFFFFFFFFFFEULL, 2);
return 0;
}There was a problem hiding this comment.
Well, if the inputs are always going to be i32 from webassembly, I don't think the check as described would serve any purpose. I though the NOT_IN_BOUNDS was intended to check whether we were indeed accessing memory withing the webassembly block.
There was a problem hiding this comment.
Well, if the inputs are always going to be i32 from webassembly, I don't think the check as described would serve any purpose. I though the
NOT_IN_BOUNDSwas intended to check whether we were indeed accessing memory withing the webassembly block.
Agree. It's defensive: I.e. what happens if somehow for some reason uint64_ts are passed into the macro and they overflow the integer.
Thanks @RichardAH, this addressed the first issue. It also doesn't build with stdc++17, so I switched to stdc++20. Now I have a boost issue: The doc still states that boost 1.70 is required, and I'm using 1.71. |
I only build on linux, so I'm not sure 😬 Is this specific to the Hooks PR or does rippled in general cause this? |
The current version of rippled from XRPL (1.9.2) builds fine with stdc++17 and boost 1.71. Not sure where these new requirements originate from. I'm trying to build the Hooks PR with C++20 and boost 1.75, and I still have errors. It looks like it tries to import boost outcome as a C++ module. I'm not quite sure how to turn this off when C++20 is set. |
|
Just to stick my nose in briefly, I'd like to discourage us from using modules in C++20. There are a number of issues that still need to be worked out, particularly with cross-platform applications. There are folks actively working on resolving these problems in C++23. |
I've never tried building Hooks on Windows. I will try now and see if I can shed some light on this. |
@greg7mdp @RichardAH I haven't been keeping up with this conversation, but I happened to notice this comment, and tried to build for the first time on my Windows box. It failed to even get to rippled code, choking on the One option is to just document the dependency. Optimally, it would be great if rippled's CMake config could treat LLVM as yet another external project. |
|
@RichardAH what's your preferred path forward for this PR? |
|
We are launching hooks on a sidechain so this can close |
|
By popular demand Hooks PR is being reopened Next steps:
We will provide developer time on points 3 and 4. We expect Ripple to provide dev time on 2 and 4. Let’s go! |
There was a problem hiding this comment.
The target of this PR is the hooks branch, and this approval indicates this PR can be merged to (only) that branch as-is as a step along the path of developing Hooks. I have not code reviewed this, and these changes will not be merged to the mainline development branch (develop) until at least the following have been completed:
(Listed below in no particular order)
- Merged/tested with the latest code in
develop, including any additional testing/changes appropriate due to potential interactions with other amendments/functionality - Figure out how to build this on Windows for development
- Improved APIs
- Resolution of any known issues
- Resolution of all TODOs
- Security audits/reviews
|
Suggested commit message: |
The Hooks Amendment adds smart contract-like functionality to the XRP Ledger. With this amendment users may choose to install small, efficient web assembly binaries onto their XRPL Accounts using the new
SetHooktransactor. These binaries allow for custom logic about which transactions are accepted (or rejected) and can also emit new transactions and manage custom information called Hook State.These early blogs describe the premise of Hooks:
Getting Started
Hooks Builder
A developer-centric Hooks IDE for your browser, connected to the testnet. (No download is required.)
Hooks Transaction Explorer
A custom technical explorer for the testnet.
Hooks V2 Staging Testnet
Consists of 9 nodes, peering information: https://xumm.notion.site/Hooks-V2-staging-net-info-XLS20-518fa261c5cd49d2bcb89a5b9e7bef05.
Hooks Documentation
Is not present on github, lives exclusively on redme.io.
Test Plan
https:/XRPL-Labs/xrpld-hooks/tree/develop/hook
Notes
Future Tasks