Parallelize transaction validation for transaction sets. - #5449
Conversation
While the current logic intertwines reads and writes, in fact it can be cleanly separated into a read-only validation step, and a sequential commit step that simply bumps the sequence numbers and removes pre-authorized tx signers. This is possible because that while the writes change the entries that take part in validation, none of these changes are relevant during the validation. Specifically, sequence number bump is only observable by a single transaction (the one that has the respective account as a source), and the pre-authorized tx signer by definition belongs to a single transaction. There is also a subtle caveat to the latter operation: it increases the available balance of the signer owner (or its sponsor), but since at the pre-apply time the fees have already been charged, we're only checking that the account available balance is non-negative, which is an invariant that must always hold in the current protocol. The change is not protocol-gated because it's not a protocol change for the *current* protocol. It was technically a protocol change prior to p26 where we had a bug that actually did allow overcharging the fee bump source accounts and thus making their available balance to go negative. However, the bug has been fixed without the behavior ever triggering on-chain, and thus this replay-only behavior change should be non-observable. This change significantly speeds up the pre-apply step. On the local high TPL benchmarks I'm getting 30-60ms improvement locally compared to the main branch version.
This affects both nominating a new transaction set, and validating the incoming transaction set. Thanks to the fact that most of the time we're either applying the ledger, or validating a transaction set, we can use the efficient CPU-pinned batch executor for this. This speeds up the invalid transaction trimming step by ~20ms on large benchmarks, and in general should increase the transaction validation step proportionally to the number of cores.
12192a2 to
443a838
Compare
| virtual LedgerHeaderWrapper getLedgerHeader() const = 0; | ||
| // Returns the pointer to Soroban network config snapshot associated with | ||
| // this view, or nullptr when the view doesn't carry one. | ||
| virtual SorobanNetworkConfig const* getSorobanNetworkConfig() const = 0; |
There was a problem hiding this comment.
I'm not a fan of the SorobanNetworkConfig changes here, just because the optional is a footgun. We already have some places where the config is an optional due to replay of older protocol versions, now this can mean you just happen to have a different ledger state view. I don't think this is necessary, can we not just have the main thread get the soroban network config then pass a copy/reference to be used by the parallel workers instead?
| // | ||
| // Lookups are first attempted in the LTX *newest version* only (which is thread | ||
| // safe as long as we don't mutate the LTX), and only then in the LCL view. | ||
| class SorobanPreApplyLedgerView : public AbstractLedgerView |
There was a problem hiding this comment.
This class seems sketchy to me for a few reasons. First, we're directly calling LedgerTxn functions in a multithreaded environment. We don't have any assertions that come with regular LedgerTxn functions, like that there are no children invalidating the current map, the main thread isn't calling mutating functions like load, etc.
I'd rather just use the mGlobalEntryMap struct here instead of a new viewer object, or if the interface is too tricky, something similar. Basically, I want to keep the pattern where a single thread always calls into Ltx and prepares some const, thread safe mappiong of relevant hot state for the multithreaded RO consumer.
There was a problem hiding this comment.
First, we're directly calling LedgerTxn functions in a multithreaded environment.
We're just calling the thread-safe getNewestBelowRoot here. We could add another layer of abstraction on top of LTX, but I'm not sure if that wins us much. This view is specialized and it seems unlikely that anyone would accidentally use it outside of the intended context. LTX could be const here though, which removes concerns about internal mutation. I would not worry about the external mutation; with parallelization we're introducing a lot of blocking parallel steps which are easy to argue about, and I don't see much point in making the code more complicated to account for the hypothetical off-thread mutation. Basically we can treat BatchExecutor steps as sequential steps from the overall app standpoint, we only need to make sure that the worker threads do not race between each other.
I'd rather just use the mGlobalEntryMap struct here instead of a new viewer object, or if the interface is too tricky, something similar.
I'm not sure what do you mean, this is a view that we need to pass to CheckValidLedgerViewWrapper. I don't think we need to create yet another interface for that.
| #endif | ||
| ~LedgerTxnReadOnly() override; | ||
| LedgerHeaderWrapper getLedgerHeader() const override; | ||
| SorobanNetworkConfig const* getSorobanNetworkConfig() const override; |
There was a problem hiding this comment.
This seems like a bit of a footgun, since in the tx subysystem a null NetworkConfig could be for historical protocol replay, or if our LedgerView just happens to by of the wrong type. Can we just keep getLastClosedSorobanNetworkConfig as is, call it from the main thread, and pass in a reference/copy to the multithread workers?
There was a problem hiding this comment.
I think it's appropriate for the ledger view to provide access to the config, and . But also upon revisiting the code it seems weird that we also load the config from the snapshot. I wouldn't dwell on LedgerTxnReadOnly specifically, as we use it as kind of a hack in tests. In prod code paths LedgerTxnReadOnly is not really used as a full AbstractLedgerView, and it probably doesn't have to inherit the full interface for what it does. I'll need to experiment a bit to figure that out. Ultimately I feel like this points at some issues with the current design. I would prefer resolving these instead of shoving the network config as yet another side input for the transaction machinery.
2 participants
Description
This affects both nominating a new transaction set, and validating the incoming transaction set. Thanks to the fact that most of the time we're either applying the ledger, or validating a transaction set, we can use the efficient CPU-pinned batch executor for this.
This speeds up the invalid transaction trimming step by ~20ms on large benchmarks, and in general should increase the transaction validation step proportionally to the number of cores.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)