Protocol 19 - CAP-0021 and CAP-0040 #3377

sisuresh · 2022-03-17T19:36:54Z

Description

Resolves #3365 and #3366.

This PR contains two commits that can be ignored (both prefixed with TEST REFACTOR - IGNORE ). Once #3362 is merged, these commits can be deleted.

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

MonsieurNicolas

only did a partial review, found a few bugs

src/herder/TransactionQueue.cpp

MonsieurNicolas

I completed my first review pass, I will do another one after this gets rebased on top of master when we have merged the tests refactor (that adds quite a bit of noise)

src/herder/TransactionQueue.cpp

src/herder/test/TransactionQueueTests.cpp

src/test/TxTests.cpp

src/transactions/TransactionFrame.cpp

MonsieurNicolas · 2022-03-19T01:07:58Z

src/transactions/TransactionFrame.cpp

+        return true;
+    }
+
+    if (mEnvelope.type() == ENVELOPE_TYPE_TX &&


you don't need to check for mEnvelope.type() == ENVELOPE_TYPE_TX here (getMinSeqNum takes care of it)

jonjove

Reviewed up to "Updated tx queue and txset validation". I think the next part is going to take quite a bit more time, so I wanted to add the comments I already had.

src/transactions/TransactionBridge.cpp

src/transactions/TransactionFrame.cpp

src/transactions/TransactionFrame.h

src/transactions/TransactionUtils.cpp

src/transactions/TransactionFrame.cpp

src/transactions/test/SponsorshipTestUtils.cpp

src/transactions/BumpSequenceOpFrame.cpp

src/herder/TransactionQueue.cpp

src/herder/TxSetFrame.cpp

src/transactions/SignatureUtils.cpp

src/transactions/SetOptionsOpFrame.cpp

src/herder/TransactionQueue.cpp

src/crypto/SignerKey.cpp

sisuresh · 2022-04-05T18:26:06Z

src/transactions/TransactionFrame.cpp

+        else
+        {
+            ltxe.currentGeneralized().maxSeqNumToApplyEntry().maxSeqNum =
+                getSeqNum();


This assumes processFeeSeqNum is called with increasing sequence numbers per source account. This is true at the moment, but it might be a good idea to use std::max or throw if the sequence numbers aren't increasing.

I agree... at the same time, why not just use max now then as it makes it clear that this is the max over all txs for that source?

src/xdr/Stellar-ledger-entries.x

src/transactions/TransactionFrame.h

src/util/ProtocolVersion.h

src/transactions/TransactionFrame.cpp

src/transactions/SignatureUtils.cpp

MonsieurNicolas · 2022-04-06T02:09:14Z

src/transactions/TransactionFrame.cpp

+        else
+        {
+            ltxe.currentGeneralized().maxSeqNumToApplyEntry().maxSeqNum =
+                getSeqNum();


I agree... at the same time, why not just use max now then as it makes it clear that this is the max over all txs for that source?

src/herder/test/HerderTests.cpp

src/transactions/TransactionFrame.cpp

src/test/TxTests.cpp

src/transactions/TransactionFrame.cpp

src/transactions/TransactionFrame.h

src/main/Config.cpp

MonsieurNicolas

getting close!

src/herder/test/HerderTests.cpp

src/crypto/SecretKey.cpp

src/transactions/TransactionFrame.cpp

src/crypto/SecretKey.h

src/crypto/test/CryptoTests.cpp

src/ledger/LedgerManagerImpl.cpp

jonjove

I've reviewed everything but the tests, and this generally looks good. I will spend some time reviewing the tests in the next few days, but I didn't want to hold up the whole PR over that.

My main concern is that the KeyUtils changes are so difficult to understand and evaluate. It's really hard to follow every path down and assess that only sane things are happening. I know that #3397 is already about this, but I wanted to reiterate that it makes me uneasy. We should strongly consider addressing this as soon as possible.

jonjove · 2022-04-13T15:12:30Z

src/ledger/LedgerManagerImpl.cpp

@@ -1128,8 +1128,12 @@ LedgerManagerImpl::processFeesSeqNums(
                }
                else
                {
-                    throw std::runtime_error(
-                        "found unexpected MAX_SEQ_NUM_TO_APPLY");
+                    ltxe.currentGeneralized()


I'm a bit skeptical about this. This would_definitely be a bug, and hiding bugs is scary because you don't know the root cause. Is it something mild or something bad? For example, imagine that there were a bug that left MAX_SEQ_NUM_TO_APPLY in LedgerTxn between ledgers. Then nodes which were started at different times wouldn't necessarily get the same result. It's probably better to throw.

Yeah that's a valid point. I removed that fixup commit.

MonsieurNicolas · 2022-04-14T00:56:29Z

r+ 0e5c9e6

sisuresh requested a review from jonjove March 17, 2022 19:36

MonsieurNicolas requested changes Mar 18, 2022

View reviewed changes

src/herder/TransactionQueue.cpp Outdated Show resolved Hide resolved

src/herder/TransactionQueue.cpp Show resolved Hide resolved

src/herder/TransactionQueue.cpp Show resolved Hide resolved

src/herder/TransactionQueue.cpp Show resolved Hide resolved

MonsieurNicolas requested changes Mar 19, 2022

View reviewed changes

jonjove suggested changes Mar 22, 2022

View reviewed changes

sisuresh mentioned this pull request Mar 24, 2022

CAP-0021 updates stellar/stellar-protocol#1155

Merged

sisuresh force-pushed the cap21-40-2 branch from e921785 to 95d896a Compare March 24, 2022 19:51

sisuresh commented Mar 24, 2022

View reviewed changes

src/transactions/BumpSequenceOpFrame.cpp Show resolved Hide resolved

jonjove suggested changes Mar 25, 2022

View reviewed changes

Shaptic mentioned this pull request Mar 26, 2022

all: Update Protocol 19 XDR to the latest stellar/go#4308

Merged

2opremio mentioned this pull request Mar 31, 2022

horizon: Set up protocol 19 integration tests infrastructure stellar/go#4312

Merged

sisuresh force-pushed the cap21-40-2 branch 2 times, most recently from f0ea865 to eb70364 Compare March 31, 2022 20:22

sisuresh commented Apr 1, 2022

View reviewed changes

src/herder/TransactionQueue.cpp Show resolved Hide resolved

sisuresh force-pushed the cap21-40-2 branch from eb70364 to 6323442 Compare April 5, 2022 01:17

sisuresh commented Apr 5, 2022

View reviewed changes

src/crypto/SignerKey.cpp Show resolved Hide resolved

sisuresh commented Apr 5, 2022

View reviewed changes

MonsieurNicolas requested changes Apr 6, 2022

View reviewed changes

MonsieurNicolas reviewed Apr 6, 2022

View reviewed changes

src/test/TxTests.cpp Outdated Show resolved Hide resolved

MonsieurNicolas reviewed Apr 6, 2022

View reviewed changes

src/transactions/TransactionFrame.cpp Outdated Show resolved Hide resolved

sisuresh force-pushed the cap21-40-2 branch from bf50192 to e9500ce Compare April 7, 2022 20:28

MonsieurNicolas reviewed Apr 8, 2022

View reviewed changes

src/transactions/TransactionFrame.h Outdated Show resolved Hide resolved

MonsieurNicolas reviewed Apr 8, 2022

View reviewed changes

src/main/Config.cpp Outdated Show resolved Hide resolved

MonsieurNicolas requested changes Apr 8, 2022

View reviewed changes

sisuresh added 6 commits April 8, 2022 12:29

Add XDR for CAP-0021

d1f6304

Add and use getTimeBounds method

70b6e1c

Add ProtocolVersion 19

ceafd80

Update seqLedger and seqTime

10e3b4a

Validate ledger bounds

0811bcf

Relax seq num check

78711e8

sisuresh added 4 commits April 8, 2022 12:29

Validate minSeqAge and minSeqLedgerGap

39da906

Take v3 ext into account in test utils

c4443bc

Take v2 ext into account in applyCheck

2523ef9

Updated tx queue and txset validation

9d60b80

sisuresh force-pushed the cap21-40-2 branch 2 times, most recently from 55bed5f to ce547ea Compare April 9, 2022 03:51

leighmcculloch mentioned this pull request Apr 9, 2022

Add support for protocol 19 and P strkey xdrpp/stc#15

Closed

sisuresh force-pushed the cap21-40-2 branch 2 times, most recently from cf891f2 to 6e3997a Compare April 11, 2022 15:23

sisuresh commented Apr 11, 2022

View reviewed changes

src/ledger/LedgerManagerImpl.cpp Show resolved Hide resolved

sisuresh force-pushed the cap21-40-2 branch from e6c0be4 to 77fef1a Compare April 12, 2022 01:18

jonjove reviewed Apr 13, 2022

View reviewed changes

sisuresh added 9 commits April 13, 2022 09:17

Add CAP-0021 tests

4fa3a12

Add xdr for CAP-0040

7efc6e4

Add CAP-0040 signer

56d2755

Add CAP-0040 tests

108d021

check largest tx seq num in MergeOp

ea2cbaa

format xdr files

11d39a5

KeyUtils changes for payload signer support

518eabc

Only return LedgerEntry in getDelta

e98a36a

incorporate payload signers into alternative signer tests

5053491

sisuresh force-pushed the cap21-40-2 branch from 77fef1a to 0def2aa Compare April 13, 2022 20:51

sisuresh added 4 commits April 13, 2022 13:55

Add extra signer tests in TxEnvelopeTests

1e1097a

Update test tx meta next

3367c55

Update test tx meta current

0da09fe

scale ed25519 signers in extraSigners

0e5c9e6

sisuresh force-pushed the cap21-40-2 branch from 0def2aa to 0e5c9e6 Compare April 13, 2022 20:55

latobarita merged commit 27c798e into stellar:master Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocol 19 - CAP-0021 and CAP-0040 #3377

Protocol 19 - CAP-0021 and CAP-0040 #3377

sisuresh commented Mar 17, 2022

MonsieurNicolas left a comment

MonsieurNicolas left a comment

MonsieurNicolas Mar 19, 2022

jonjove left a comment

sisuresh Apr 5, 2022

MonsieurNicolas Apr 6, 2022

sisuresh Apr 7, 2022

MonsieurNicolas Apr 6, 2022

MonsieurNicolas left a comment

jonjove left a comment

jonjove Apr 13, 2022

sisuresh Apr 13, 2022

MonsieurNicolas commented Apr 14, 2022

Protocol 19 - CAP-0021 and CAP-0040 #3377

Protocol 19 - CAP-0021 and CAP-0040 #3377

Conversation

sisuresh commented Mar 17, 2022

Description

Checklist

MonsieurNicolas left a comment

Choose a reason for hiding this comment

MonsieurNicolas left a comment

Choose a reason for hiding this comment

MonsieurNicolas Mar 19, 2022

Choose a reason for hiding this comment

jonjove left a comment

Choose a reason for hiding this comment

sisuresh Apr 5, 2022

Choose a reason for hiding this comment

MonsieurNicolas Apr 6, 2022

Choose a reason for hiding this comment

sisuresh Apr 7, 2022

Choose a reason for hiding this comment

MonsieurNicolas Apr 6, 2022

Choose a reason for hiding this comment

MonsieurNicolas left a comment

Choose a reason for hiding this comment

jonjove left a comment

Choose a reason for hiding this comment

jonjove Apr 13, 2022

Choose a reason for hiding this comment

sisuresh Apr 13, 2022

Choose a reason for hiding this comment

MonsieurNicolas commented Apr 14, 2022