Add tests to trigger witness limit error paths #1917
Conversation
| pver := uint32(70001) | ||
| txVer := uint32(1) | ||
|
|
||
| var createTxnWithManyWitnessItems = func() []byte { |
There was a problem hiding this comment.
Doesn't look to use many of the variables closed over, so I think we can just make this a regular function and then call it below.
Pull Request Test Coverage Report for Build 4331080812
馃挍 - Coveralls |
otech47
force-pushed
the
add-tests-witness-limits
branch
from
94a9f9a
to
0b59cb7
Compare
Most of the existing tests use hard-coded byte arrays for various tests so simulating a txn with a theoretical limit of 4M bytes of witness data required a function to mock 4M 1-byte witness items
| @@ -778,6 +779,149 @@ func TestTxWitnessSize(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // TestTxWitnessLimits performs negative tests to ensure decoding | |||
| // serialized transactions with witness data triggers proper error paths. | |||
| func TestTxWitnessLimits(t *testing.T) { | |||
There was a problem hiding this comment.
thinking more about it... if these limit checks are already happening deeper in consensus logic, I wonder if it doesn't make more sense to simply forget about witness limit checks and just do a <4MB size check on the entire txn? if these checks at a parsing level are really more about memory exhaustion safeguards, there shouldn't be a need to look for the witness specifically?
There was a problem hiding this comment.
, I wonder if it doesn't make more sense to simply forget about witness limit checks and just do a <4MB size check on the entire txn?
That would certainly be the most conservative check. The issue with doing it on this layer though, is the possibility of future interactions that increase this value (say an ext block in the future or w/e). On the bitcoind side, they have such limits in place still, but it's around 32 MB or so (iirc the max p2p payload). So if we end up upgrading Bitcoin far in the future to 64 MB blocks or w/e, then that would cause a fork with these old clients.
|
Approved the CI run. |
To address #1915, I noticed the testing suite does not cover the possibility for very large transactions to trigger errors meant for denial-of-service/memory exhaustion protection.
Parsing code is not consensus code per-say, but it's a grey area if you define consensus code as "anything that could halt execution before validation rules can be reached" so we should be wary of code that parses bytes straight from the wire since it could cause consensus failure
The
msgtx_testsuite also has not been updated since the taproot consensus upgrade so it's worth looking into what kinds of changes might make the wire parsing module more robust for P2TR spends. For example, all existing tests are only usingBaseEncodingat the moment so adding redundant versions of those tests that use theWitnessEncodingmay be useful?Since refactoring such critical code is incredibly difficult and risky, this PR attempts to at least take the basic approach of adding more test cases to improve coverage.
I hope to add at least a few more cases before taking it out of draft state (and open to other suggestions)
too many witness items to fit into max message size...<scriptSize> is larger than the max allowed size...too many input transactions to fit into max message size...(withWitnessEncoding)too many output transactions to fit into max message size...(withWitnessEncoding)PS: I'm new to golang so be gentle/ruthless if I am using any anti-patterns 馃珷