-
Notifications
You must be signed in to change notification settings - Fork 970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only interpret TxSetFrame
with correct ledger state
#3977
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a helpful change! Just a few comments about raw pointers and TxSetFrame
construction paths, but overall looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think this is definitely a tricky area, so would be great to ensure we have a proper contract for all the different tx set states, and add invariance where possible to prevent issues related to invalid local state in the future.
src/herder/HerderImpl.cpp
Outdated
// Even if we're in protocol 20, still check for number of phases, in case | ||
// we're dealing with the upgrade ledger that contains old-style transaction | ||
// set | ||
if (txSet->numPhases() > static_cast<size_t>(TxSetFrame::Phase::SOROBAN) && | ||
mSorobanTransactionQueue) | ||
if (mSorobanTransactionQueue != nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if (mSorobanTransactionQueue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why though? I'm not a fan of pointer-to-bool casts as it's not immediately clear what you're dealing with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly for consistency with the rest of the codebase using mSorobanTransactionQueue. I don't feel strongly though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the updates are easier to review, thanks! Left a few questions on lifetime of tx set cache items, and some transaction frame fee-related concerns
// Before even trying to validate and apply a TxSetFrame it has | ||
// to be interpreted and prepared for apply using the ledger state | ||
// this TxSetFrame refers to. This is typically performed by | ||
// `prepareForApply` method. | ||
class TxSetFrame : public NonMovableOrCopyable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the caching got moved out of TxSetFrame, shall we rename is to TxSetXDRFrame or something that suggests that it's just a wrapper around XDR? I think it'd help with readability and make it easier to spot places that should be using ApplicableTxSetFrame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I would probably do that as a followup to reduce the scope of changes a bit. I'm also not a fan of the static factory functions that return a different type, so I'd refactor that as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. let's an open an issue about this to we don't forget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #4017
7d69623
to
4383758
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Much easier to reason about without the caches. Just a few small things I noticed
// Before even trying to validate and apply a TxSetFrame it has | ||
// to be interpreted and prepared for apply using the ledger state | ||
// this TxSetFrame refers to. This is typically performed by | ||
// `prepareForApply` method. | ||
class TxSetFrame : public NonMovableOrCopyable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good. let's an open an issue about this to we don't forget
xdr::xvector<TransactionEnvelope> const& txs, | ||
bool useBaseFee, std::optional<int64_t> baseFee, | ||
Phase phase); | ||
TxSetFrame::Phase phase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was like this before your change, but can we make computeTxFees
const, since we're cleaning up the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/herder/TxSetFrame.cpp
Outdated
txOps = tx.v1().tx.operations.size(); | ||
break; | ||
case ENVELOPE_TYPE_TX_FEE_BUMP: | ||
txOps = tx.feeBump().tx.innerTx.v1().tx.operations.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fee bumps add an extra op to the inner tx op count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
size_t txOps = 0; | ||
switch (tx.type()) | ||
{ | ||
case ENVELOPE_TYPE_TX_V0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting warnings about other envelope types that aren't handled. Should we add a default and throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used for logging actually, so I would rather handle unknown/incorrect envelopes gracefully. I've renamed the function for clarity as well.
protocolVersion, sorobanResources(), | ||
static_cast<uint32_t>(xdr::xdr_size(mEnvelope)), 0, sorobanConfig, | ||
cfg)); | ||
static_cast<uint32_t>(xdr::xdr_size(mEnvelope)), 0, sorobanConfig, cfg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: safer would be to do getResources(false).getVal(TX_BYTE_SIZE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…ger state. Most of the time we just process tx sets as XDR blobs. We only need to interpret tx sets in a few cases (when validating the SCP nominees and applying the final tx set). These cases now have to explicitly use a different tx set interface `ApplicableTxSetFrame` which can only be created from a TxSet that points at current LCL. Also remove caching of Soroban resource fee from tx frame. Fee caching proved to be a major footgun that might introduce divergence in asynchronous flows (any type of catchup).
r+ fe7bbcf |
Description
Resolves #3958
Refactor
TxSetFrame
to distinguish between operations that are dependent on the ledger state vs those that aren't.Most of the time we just process tx sets as XDR blobs. We only need to further interpret tx sets in a few cases (when validating the SCP nominees and applying the final tx set). These cases now have to explicitly use a different tx set interface
ResolvedTxSetFrame
which can only be created when TxSet points at current LCL.Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)