Skip to content

Update sequence number processing#1519

Closed
MonsieurNicolas wants to merge 8 commits intostellar:masterfrom
MonsieurNicolas:seqNumProcessingV10
Closed

Update sequence number processing#1519
MonsieurNicolas wants to merge 8 commits intostellar:masterfrom
MonsieurNicolas:seqNumProcessingV10

Conversation

@MonsieurNicolas
Copy link
Copy Markdown
Contributor

resolves #1445

this PR changes the way we update the sequence number in the account in preparation for the BumpSeqOperation:

  • it moves sequence number processing from the pre-apply stage (where fees are processed) to the actual transaction apply time (protocol v10)
  • it also changes the meta format for all versions. generating a "tx level side effect" meta. this field only gets updated starting at protocol 10, but I figured it was better to move to the new meta in all cases to avoid carrying over the complexity in future versions of Horizon (old meta support can be removed later)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the idea of commonValid function changing anything. The name implies that it is pure function, but now it is not. I think we should drop idea of having common valid method with applying parameter and try to do something different here.

We call commonValid from two places:

  • checkValid to check validity before accepting transaction and
  • apply to check validity when applying transaction

And it the middle of it is fee and seq number processing (but only if applying is true!).
If we split that in 3 steps code could became clearer:

  • checkPreValid (needs better name - it is commonValidPreSeqNum now)
  • checkSeqNumValid
  • checkPostValid (needs better name and additional parameter balanceAfter)

Then checkValid could do something like this:

if (!checkPreValid()) return false;
if (!checkSeqNumValid()) return false;
if (!checkPostValid(mSigningAccount->getAccount().balance - mEnvelope.tx.fee)) return false;

And apply could do something like this:

if (!checkPreValid()) return false;
if (lm.getCurrentLedgerVersion() >= 10)
{
  if (!checkSeqNumValid()) return false;
  setSeqNum() and storeChange();
}
if (!checkPostValid(lm.getCurrentLedgerVersion() > 8 ? mSigningAccount->getAccount().balance - mEnvelope.tx.fee : mSigningAccount->getAccount().balance)) return false;

I think it will be much cleared that all those conditions with applying and account change inside validCommon method.

now that we use "CreateTestApplication", the version used by default is "latest"
Regardless of protocol version to allow consumers (like Horizon)
to move to it 100% in the future
allows to perform more checks directly there instead of having
the caller perform those;
also removed some exceptions that are not relevant
@MonsieurNicolas
Copy link
Copy Markdown
Contributor Author

@vogel I updated based on your feeback. I ended up changing back "Check" to only do checks (and moved the side effect code outside), it avoids introducing new functions and semantics that make it hard to work across protocol versions

@vogel
Copy link
Copy Markdown
Contributor

vogel commented Feb 1, 2018

I like the changes! Now we only need to do full catchup with that ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decouple sequence number consumption out of fee processing

2 participants