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 a test parsing transaction with a huge witness #1359
Conversation
This transaction broke past versions of `rust-bitcoin` and LND so this adds a test to avoid reintroducing the problem in the future. See also romanz/electrs#783
IIUC, the failure is due to
and indeed the witness definition has changed between 0.27.1 and 0.28.0: diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs
index 4ecac18..445da7d 100644
--- a/src/blockdata/transaction.rs
+++ b/src/blockdata/transaction.rs
@@ -196,7 +198,7 @@ pub struct TxIn {
/// Encodable/Decodable, as it is (de)serialized at the end of the full
/// Transaction. It *is* (de)serialized with the rest of the TxIn in other
/// (de)serialization routines.
- pub witness: Vec<Vec<u8>>
+ pub witness: Witness
} The fix was introduced in 2fd0125. |
Damn, ideally this kind of thing would have been addressed through #1023. |
Ah, this is about rust-bitcoin 0.28, the above landed in 0.29 (according to github). So hopefully we have a substantially more robust answer to these kinds of bugs in 0.29 already. |
AFAIU even 0.28 is unaffected. This PR just adds test out of caution. |
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.
ACK, this is an important test, as this tx did break production systems
I could not foresee the impact when I opened #997. It also lists other potential ways in which block decoding could fail, but all of them required the miner to specifically craft a non-standard tx. Perhaps, this is a lesson for us for back-porting all bug fixes related to decoding/encoding even if they "require miner assistance to deploy". |
I have created #1360 as a backport for 0.28 users. |
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.
ACK d6ca7e4
a0489d4 fuzz: use travis-fuzz.sh in CI (Andrew Poelstra) 4c6f9b3 fuzz: remove mysteriously-not-necessary quotes from gh action script (Andrew Poelstra) 7baa21c fuzz: disable features in honggfuzz (Andrew Poelstra) e003e57 Add `consensus_decode_from_finite_reader` optimization (Dawid Ciężarkiewicz) Pull request description: Backport for #1023. Required for #997. Addresses issues like #1359 ACKs for top commit: tcharding: ACK a0489d4 TheBlueMatt: ACK a0489d4. Tree-SHA512: 9145d9666e35ae77598aaecf89222c7234637b57ded39b69fbb93ee9ce01c6d7c938b36a2d86319ba84155f2424e524386593d6c0d7af449be6bd118f729fd64
This transaction broke past versions of
rust-bitcoin
and LND so this adds a test to avoid reintroducing the problem in the future.See also romanz/electrs#783
I'm publishing this immediately for research purposes. I can clean it up later if required (low on time rn) or we may even not merge it if there is a better test.