From f7911d78dbf4f57f8fa1ee5fcc3c90e38e9b1004 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Wed, 28 Aug 2024 17:00:04 -0400 Subject: [PATCH 1/6] Derive looser, but more principled, checks of txn max size --- node/node_test.go | 43 +++++++++++++++++++++++++++++++++++++++++-- protocol/tags.go | 22 ++++++++++++++++++---- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/node/node_test.go b/node/node_test.go index df72a699bb..0136d347bd 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -33,9 +33,11 @@ import ( "github.com/algorand/go-algorand/agreement" "github.com/algorand/go-algorand/config" "github.com/algorand/go-algorand/crypto" + csp "github.com/algorand/go-algorand/crypto/stateproof" "github.com/algorand/go-algorand/data/account" "github.com/algorand/go-algorand/data/basics" "github.com/algorand/go-algorand/data/bookkeeping" + "github.com/algorand/go-algorand/data/stateproofmsg" "github.com/algorand/go-algorand/data/transactions" "github.com/algorand/go-algorand/logging" "github.com/algorand/go-algorand/network" @@ -805,11 +807,48 @@ func TestMaxSizesCorrect(t *testing.T) { require.Equal(t, ppSize, protocol.ProposalPayloadTag.MaxMessageSize()) spSize := uint64(stateproof.SigFromAddrMaxSize()) require.Equal(t, spSize, protocol.StateProofSigTag.MaxMessageSize()) - txSize := uint64(transactions.SignedTxnMaxSize()) - require.Equal(t, txSize, protocol.TxnTag.MaxMessageSize()) msSize := uint64(crypto.DigestMaxSize()) require.Equal(t, msSize, protocol.MsgDigestSkipTag.MaxMessageSize()) + // We want to check that the TxnTag's max size is big enough, but it is + // foolish to try to be exact here. We will confirm that it is bigger that + // a stateproof txn (the biggest kind, which can only appear by itself), and + // that it is bigger than 16 times the largest transaction other than + // stateproof txn. + txTagMax := protocol.TxnTag.MaxMessageSize() + + // SignedTxnMaxSize() is an overestimate of a single transaction because it + // includes fields from all the different types of signatures, and types of + // transactions. First, we remove the aspects of the overestimate that come + // from the multiple signature types. + maxCombinedTxnSize := uint64(transactions.SignedTxnMaxSize()) + // subtract out the two smaller signature sizes (logicsig is biggest, it can *contain* the others) + maxCombinedTxnSize -= uint64(crypto.SignatureMaxSize() + crypto.MultisigSigMaxSize()) + // the logicsig size is *also* an overestimate, because it thinks each + // logicsig arg can be big, but really the sum of the args and the program + // has a max size. + maxCombinedTxnSize -= uint64(transactions.EvalMaxArgs * config.MaxLogicSigMaxSize) + + // maxCombinedTxnSize is still an overestimate because it assumes all txn + // type fiels can be in the same txn. That's not true, but it provides an + // upper bound on the size of ONE transaction, even if the txn is a + // stateproof, which is big. Ensure our constant is big enough to hold one. + require.Greater(t, txTagMax, maxCombinedTxnSize) + + // we actually have to hold 16 txns, but in the case of multiple txns in a + // group, none can be stateproofs. So derive maxMinusSP, which is a per txn + // size estimate that excludes stateproof fields. + spTxnSize := uint64(csp.StateProofMaxSize() + stateproofmsg.MessageMaxSize()) + maxMinusSP := maxCombinedTxnSize - spTxnSize + require.Greater(t, txTagMax, 16*maxMinusSP) + // when we do logisig pooling, 16*maxMinusSP may be a large overshoot, since + // it will assume we can have a big logicsig in _each_ of the 16. It + // probbaly won't matter, since stateproof will still swamp it. But if so, + // remove 15 * MaxLogicSigMaxSize. + + // but we're not crazy. whichever of those is bigger - we don't need to be twice as big as that + require.Less(t, txTagMax, 2*max(maxCombinedTxnSize, 16*maxMinusSP)) + // UE is a handrolled message not using msgp // including here for completeness ensured by protocol.TestMaxSizesTested ueSize := uint64(67) diff --git a/protocol/tags.go b/protocol/tags.go index 6cfcacd714..e8c3bf806b 100644 --- a/protocol/tags.go +++ b/protocol/tags.go @@ -84,10 +84,24 @@ const StateProofSigTagMaxSize = 6378 // Matches current network.MaxMessageLength const TopicMsgRespTagMaxSize = 6 * 1024 * 1024 -// TxnTagMaxSize is the maximum size of a TxnTag message. This is equal to SignedTxnMaxSize() -// which is size of just a single message containing maximum Stateproof. Since Stateproof -// transactions can't be batched we don't need to multiply by MaxTxnBatchSize. -const TxnTagMaxSize = 4620031 +// TxnTagMaxSize is the maximum size of a TxnTag message. The TxnTag is used to +// send entire transaction groups. So, naively, we might set it to the maximum +// group size times the maximum transaction size (plus a little bit for msgpack +// encoding). But there are several reasons not to do that. First, the +// function we have for estimating max transaction size +// (transactions.SignedTxnMaxSize())) wildly overesimates the maximum +// transaction size because it is generated code that assumes _every_ +// transaction field can be set, but each transaction type has mutually +// exclusive fields. Second, the stateproof transaction is the biggest +// transaction by far, but it can only appear as a singleton, so it would not +// make sense to multiply it by 16. Finally, we're going to pool logicsig code +// size, so while it's true that one transaction in a group could have a 16k +// logicsig, that would only be true if the other transactions had 0 bytes of +// logicsig. So we will use a bound that is a bit bigger that a txn group can +// be, but avoid trying to be precise. See TestMaxSizesCorrect for the detailed +// reasoning. + +const TxnTagMaxSize = 5_000_000 // UniEnsBlockReqTagMaxSize is the maximum size of a UniEnsBlockReqTag message const UniEnsBlockReqTagMaxSize = 67 From 95f4b744b0fa0005fe55461084ea223891a8fe36 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Thu, 29 Aug 2024 14:33:06 -0400 Subject: [PATCH 2/6] comment typo Co-authored-by: Jason Paulos --- node/node_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/node_test.go b/node/node_test.go index 0136d347bd..04087e944e 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -830,7 +830,7 @@ func TestMaxSizesCorrect(t *testing.T) { maxCombinedTxnSize -= uint64(transactions.EvalMaxArgs * config.MaxLogicSigMaxSize) // maxCombinedTxnSize is still an overestimate because it assumes all txn - // type fiels can be in the same txn. That's not true, but it provides an + // type fields can be in the same txn. That's not true, but it provides an // upper bound on the size of ONE transaction, even if the txn is a // stateproof, which is big. Ensure our constant is big enough to hold one. require.Greater(t, txTagMax, maxCombinedTxnSize) From 0747dc86d7d2ffddd63d9a40874677177d173134 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Thu, 29 Aug 2024 14:33:22 -0400 Subject: [PATCH 3/6] comment typo Co-authored-by: Jason Paulos --- node/node_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/node_test.go b/node/node_test.go index 04087e944e..c678b877ba 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -843,7 +843,7 @@ func TestMaxSizesCorrect(t *testing.T) { require.Greater(t, txTagMax, 16*maxMinusSP) // when we do logisig pooling, 16*maxMinusSP may be a large overshoot, since // it will assume we can have a big logicsig in _each_ of the 16. It - // probbaly won't matter, since stateproof will still swamp it. But if so, + // probably won't matter, since stateproof will still swamp it. But if so, // remove 15 * MaxLogicSigMaxSize. // but we're not crazy. whichever of those is bigger - we don't need to be twice as big as that From 1228c3699bdc9f967d4ac665b53f3ff56c8b2613 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Thu, 29 Aug 2024 14:33:40 -0400 Subject: [PATCH 4/6] comment typo Co-authored-by: Jason Paulos --- protocol/tags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocol/tags.go b/protocol/tags.go index e8c3bf806b..fad5d92a3b 100644 --- a/protocol/tags.go +++ b/protocol/tags.go @@ -89,7 +89,7 @@ const TopicMsgRespTagMaxSize = 6 * 1024 * 1024 // group size times the maximum transaction size (plus a little bit for msgpack // encoding). But there are several reasons not to do that. First, the // function we have for estimating max transaction size -// (transactions.SignedTxnMaxSize())) wildly overesimates the maximum +// (transactions.SignedTxnMaxSize())) wildly overestimates the maximum // transaction size because it is generated code that assumes _every_ // transaction field can be set, but each transaction type has mutually // exclusive fields. Second, the stateproof transaction is the biggest From 4e245d65acce4bb54bd35a6bd0b750be539cfa12 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Thu, 29 Aug 2024 14:46:43 -0400 Subject: [PATCH 5/6] Remove a newline --- protocol/tags.go | 1 - 1 file changed, 1 deletion(-) diff --git a/protocol/tags.go b/protocol/tags.go index fad5d92a3b..cdae9c6cdc 100644 --- a/protocol/tags.go +++ b/protocol/tags.go @@ -100,7 +100,6 @@ const TopicMsgRespTagMaxSize = 6 * 1024 * 1024 // logicsig. So we will use a bound that is a bit bigger that a txn group can // be, but avoid trying to be precise. See TestMaxSizesCorrect for the detailed // reasoning. - const TxnTagMaxSize = 5_000_000 // UniEnsBlockReqTagMaxSize is the maximum size of a UniEnsBlockReqTag message From 960c6c04c9cf473610a88cf02911c3455125a9e9 Mon Sep 17 00:00:00 2001 From: John Jannotti Date: Tue, 3 Sep 2024 13:43:33 -0400 Subject: [PATCH 6/6] wow, fix the test of a test --- protocol/tags_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/protocol/tags_test.go b/protocol/tags_test.go index 137bf4e3f7..69c3146cf9 100644 --- a/protocol/tags_test.go +++ b/protocol/tags_test.go @@ -169,6 +169,10 @@ func TestMaxSizesTested(t *testing.T) { } for _, tag := range constTags { + if tag == "TxnTag" { + // TxnTag is tested in a looser way in TestMaxSizesCorrect + continue + } require.Truef(t, tagsFound[tag], "Tag %s does not have a corresponding test in TestMaxSizesCorrect", tag) } }