-
Notifications
You must be signed in to change notification settings - Fork 36.9k
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
Fail in DecodeHexTx if there is extra data at the end #9634
Conversation
I had a reason why I needed to check that a transaction was one, only one, and exactly one transaction when writing some stuff that used RPC to do transaction manipulation, but, honestly I cant remember what it was now....anyway, still like this change. utACK f1c7bad (can I do that if my name is on it?) |
Yeah, I wasn't sure which case was this fixing, but I think it is a fix.
I think it's more important that you do precisely because I'm signing the commit in your name (with my key). I could replace these small changes with a backdoor over 90k loc, and keep your name in the commit. utACK I think very much needed and of course appreciated, as well as the additional context. |
utACK f1c7bad I ran into this when testing segwit coinbases. I don't remember the details exactly, but I wanted to verify that the tx was well-formed, and at one point I ended up with some garbage at the end that should've caused a failure but didn't. (Obviously other things were busted too, but this caused some additional confusion). |
Does this break being able to concatenate multiple transactions to merge signatures? |
Concept ACK - DecodeHexTX can only output one transaction (data after it is effectively discarded, no pointer is returned to the tail data, etc), so it should only accept one transaction. I'd say if concatenated transactions are to be handled somewhere that'd need a function with an API that returns a list of transactions? |
I think only signrawtransaction takes as input a concatenated list of transactions, but it does not use DecodeHexTx, and does not need to deal with incomplete transaction (I haven't checked the code). |
BTW: this needs a test |
So I remembered why I needed this. If you call signrawtransaction with transactions concatenated which have different input counts, bitcoind might crash. I used this patch to test transactions before passing them to signrawtransaction when they were passed in from non-local sources. See #9650 for a fix specific for that. |
Added a test for this in #9650. |
As this is included in #9650 plus tests, and it is still a small PR, I'm closing this. |
Rebasing @TheBlueMatt's work.
I'm not sure if this qualifies as a bugfix for 0.14.