BumpSequence by MonsieurNicolas · Pull Request #1533 · stellar/stellar-core

MonsieurNicolas · 2018-02-07T00:18:07Z

This commit superseeds #1519 (that only contained the commits related to sequence number processing)

I didn't squash commits on purpose to preserve the timeline wrt various revs of the specification that led to the current implementation.

The first batch of commits updates the way we process sequence numbers

note: I only made a minor adjustment to the last commit compared to #1519

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)
notable change in test code is that we do more validations from applyCheck (previously we were skipping a bunch of validations if the failures were too early)

After that we introduce `BumpSequence`

The first couple drafts had additional failure modes/conditions on the operation.

The latest one makes BumpSequence extremely simple and instead:

allows the sequence number to jump to any arbitrary point in the future ; for this to work I implemented support for uint64 sequence numbers in SQL (which only stores signed integers so we "wrap around"). I don't see a better way to do this (other than just not use integers at all and store that column as an base32 string which may be even more confusing and requires a schema migration). The conversion from signed to unsigned may still cause problems but is in C++ a fully specified behavior (bits are the same). @nullstyle any thoughts on this?
change MergeAccount to fail if recreating the account in the same ledger (for example in a subsequent operation in the same transaction) would lead to an account with a smaller sequence number. For this to work, I had to adjust some tests that were overlooking this fact.

vogel · 2018-02-08T11:28:49Z

src/transactions/MergeTests.cpp

Won't it break catchup complete on current pubnet?

no it won't: what I did here is change the test to be explicit about what is going on by adding an extra closeLedgerOn(*app, 2, 1, 1, 2016); after setting up the original accounts.
the check REQUIRE(a1SeqNum == a1.loadSequenceNumber()) was just misleading because the reason a1SeqNum was not changing was that the setup and the transaction being tested (with all its sub-operations) were occurring during the same ledger.

vogel · 2018-02-08T11:30:05Z

src/transactions/MergeTests.cpp

if we move that 2 lines down we could get rid of line 637

but then it would be seqNumRightAfterMax and not maxSeqNum

In that case maybe remove line 637 and use maxSeqNum + 1 in 638 and 639?

vogel · 2018-02-08T11:31:46Z

src/transactions/OperationFrame.cpp

it is bad that we cannot move MANAGE_DATA_NOT_SUPPORTED_YET here as well

yeah MANAGE_DATA_NOT_SUPPORTED_YET should not have been implemented like this in the first place and now we have to keep that behavior to replay old versions of the protocol

vogel · 2018-02-08T11:32:37Z

What happened to this one?

Another thing that I also realized is that we should also probably make it fail instead of no-op:

in the context of bumpseq being the only operation in a transaction it doesn't matter
if bumpseq is one of several operations in a transaction. it allows to have safe assumptions on the workflow.

MonsieurNicolas · 2018-02-09T00:03:26Z

Q on "Why bumpSequence doesn't fail when attempting to bump backwards": I am answering in stellar/stellar-protocol#53 as it's not clear that it was dropped.

In the issue, I am also proposing moving SequenceNumber at the protocol layer to int64 across the board as the current approach is just too hacky and unsafe across languages.

@nullstyle any input especially on that last point?

nullstyle · 2018-02-09T15:43:33Z

@MonsieurNicolas at first take, I think switching to int64 for sequence numbers should be fine. I'll do a little checking around just to further confirm.

vogel · 2018-02-12T22:45:42Z

src/test/TxTests.cpp

cant we just check before loop if modified.size() == 1?

it's a fake iterator, it only implements begin and end

vogel · 2018-02-12T22:47:42Z

src/transactions/BumpSequenceOpFrame.cpp

bumping to 0 is OK (always no-op)?

0 is a valid sequence number, so yes

vogel · 2018-02-12T22:48:55Z

src/transactions/BumpSequenceTests.cpp

so this is second way to lock account, next to setting master weight to 0, nice!

no it doesn't lock the account, it just cannot be used as source account for a transaction

vogel · 2018-02-12T22:49:48Z

src/transactions/MergeOpFrame.cpp

I think a comment here explaining why this condition exists would be nice

I forgot that I wanted to get rid of this magic across the code base

MonsieurNicolas · 2018-02-13T00:46:25Z

@vogel I updated that latest commit with your suggestions

MonsieurNicolas · 2018-03-13T18:00:54Z

(rebased to current master)

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

…2017 stellar/stellar-protocol#53

…ore)

…nstead

even though sequence numbers are stored as signed integers in SQL, in which case the values just wrap around and are stored as negative numbers

This makes it compliant with the proposal dated 02-Feb-2018 stellar/stellar-protocol#53

vogel · 2018-03-21T10:43:21Z

r+ f9f9187

BumpSequence Reviewed-by: vogel

MonsieurNicolas force-pushed the bumpseqV10 branch from 00def70 to b1d9123 Compare February 7, 2018 00:19

vogel reviewed Feb 8, 2018

View reviewed changes

MonsieurNicolas force-pushed the bumpseqV10 branch from 269f145 to 6f5b771 Compare February 9, 2018 22:04

vogel reviewed Feb 12, 2018

View reviewed changes

MonsieurNicolas force-pushed the bumpseqV10 branch from 6f5b771 to cb0ce09 Compare February 13, 2018 00:45

MonsieurNicolas mentioned this pull request Mar 13, 2018

Update load generation logic #1582

Merged

MonsieurNicolas force-pushed the bumpseqV10 branch from cb0ce09 to 36efb6d Compare March 13, 2018 18:00

MonsieurNicolas force-pushed the bumpseqV10 branch 2 times, most recently from 7b6c7df to 6fcdcf6 Compare March 20, 2018 21:06

MonsieurNicolas and others added 11 commits March 20, 2018 16:49

bump to protocol v10

1165f63

added "exists" method to testAccount

03174c9

removed obsolete calls to setCurrentLedgerVersion in txEnvelopeTests

44bd9e7

now that we use "CreateTestApplication", the version used by default is "latest"

move tests for bound checking into their own sections

534caf7

Move to new meta format

10179d6

Regardless of protocol version to allow consumers (like Horizon) to move to it 100% in the future

Process sequence number during apply

69dda2e

reworked herder test to be a bit more representative of what is going on

f879d41

reworked logic for testing transactions (applyCheck)

2e67acb

allows to perform more checks directly there instead of having the caller perform those; also removed some exceptions that are not relevant

Add the BumpSequence Operation & Tests

e82be46

Fix to make BumpSequence target the source

7e5e2a0

small refactors, load account instead of direct access

805c759

JeremyRubin and others added 13 commits March 20, 2018 16:49

Refactor to use native tx_NOACCOUNT error

7506448

updated Visual Studio project with BumpSeq files

d2d7871

clang-format & renamed BUMP_SEQ -> BUMP_SEQUENCE

cbd8d6f

updated BumpSequence to be compliant with specification dated 13-Oct-…

e5a7110

…2017 stellar/stellar-protocol#53

introduce opNOT_SUPPORTED when operations are not supported (yet/anym…

7a15050

…ore)

remove BUMP_SEQUENCE_NOT_SUPPORTED_YET, use operation level failure i…

e9ed4c5

…nstead

added safeConvertToSigned utility function

1f05651

make it safe for sequence numbers to use the entire uint64 space

1082eb0

even though sequence numbers are stored as signed integers in SQL, in which case the values just wrap around and are stored as negative numbers

update BumpSequence to allow bumping through the entire uint64 space

4fe0945

This makes it compliant with the proposal dated 02-Feb-2018 stellar/stellar-protocol#53

make AccountMerge fail when the current sequence number is too far

6257cb2

This makes it compliant with the proposal dated 02-Feb-2018 stellar/stellar-protocol#53

added tests for sequence number overflow

474ece9

added missing merge test (signers)

d7a12fd

make SequenceNumber a signed integer (BumpSeq proposal 08-Feb-2018)

f9f9187

MonsieurNicolas force-pushed the bumpseqV10 branch from 6fcdcf6 to f9f9187 Compare March 20, 2018 23:51

latobarita added a commit that referenced this pull request Mar 21, 2018

Merge pull request #1533 from MonsieurNicolas/bumpseqV10

7767d3e

BumpSequence Reviewed-by: vogel

latobarita merged commit f9f9187 into stellar:master Mar 21, 2018

MonsieurNicolas mentioned this pull request Mar 21, 2018

Make update to support latest stellar-core (10.0.0 release) stellar/go-stellar-sdk#370

Closed

tsusanka mentioned this pull request May 29, 2018

Implement Stellar trezor/trezor-core#202

Closed

Teri68 approved these changes Oct 16, 2025

View reviewed changes

Conversation

MonsieurNicolas commented Feb 7, 2018

The first batch of commits updates the way we process sequence numbers

After that we introduce BumpSequence

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vogel commented Feb 8, 2018

Uh oh!

MonsieurNicolas commented Feb 9, 2018

Uh oh!

nullstyle commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MonsieurNicolas commented Feb 13, 2018

Uh oh!

MonsieurNicolas commented Mar 13, 2018

Uh oh!

vogel commented Mar 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

After that we introduce `BumpSequence`