-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests to trigger witness limit error paths #1917
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo thanks for picking this up! A bit late w/ the review, but left some initial comments. Mainly that we can do this programmatically vs sort of hard coding the bytes of a raw tx.
pver := uint32(70001) | ||
txVer := uint32(1) | ||
|
||
var createTxnWithManyWitnessItems = func() []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, 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_test
suite 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 usingBaseEncoding
at the moment so adding redundant versions of those tests that use theWitnessEncoding
may 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 🫠