Fail in DecodeHexTx if there is extra data at the end #9634

1. 
   jtimon commented at 0:48 am on January 26, 2017: contributor

   Rebasing @TheBlueMatt’s work. I’m not sure if this qualifies as a bugfix for 0.14.
2. Fail in DecodeHexTx if there is extra data at the end f1c7bad08e
3. 
   TheBlueMatt commented at 1:00 am on January 26, 2017: member

   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 f1c7bad08e299b8a686c4f0d1ee2b827d2230500 (can I do that if my name is on it?)

4. 
   jtimon commented at 1:14 am on January 26, 2017: contributor

   Yeah, I wasn’t sure which case was this fixing, but I think it is a fix.

   utACK f1c7bad (can I do that if my name is on it?)

   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.

5. 
   theuni commented at 3:08 am on January 26, 2017: member

   utACK f1c7bad08e299b8a686c4f0d1ee2b827d2230500

   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).

6. 
   gmaxwell commented at 7:18 am on January 26, 2017: contributor

   Does this break being able to concatenate multiple transactions to merge signatures?
7. 
   laanwj commented at 8:48 am on January 26, 2017: member

   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?
8. 
   sipa commented at 4:36 pm on January 26, 2017: member

   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).
9. 
   laanwj commented at 7:26 pm on January 26, 2017: member

   BTW: this needs a test
10. jonasschnelli added the label Refactoring on Jan 27, 2017
11. 
    TheBlueMatt commented at 6:47 pm on January 28, 2017: member

    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.
12. laanwj added this to the milestone 0.14.0 on Feb 2, 2017
13. 
    TheBlueMatt commented at 9:27 pm on February 2, 2017: member

    Added a test for this in #9650.
14. 
    jtimon commented at 10:32 pm on February 2, 2017: contributor

    As this is included in #9650 plus tests, and it is still a small PR, I’m closing this.
15. jtimon closed this on Feb 2, 2017

    ---

16. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon TheBlueMatt theuni gmaxwell laanwj sipa

Labels
Refactoring

Milestone
0.14.0