Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Jan 26, 2017

Rebasing @TheBlueMatt's work.
I'm not sure if this qualifies as a bugfix for 0.14.

@TheBlueMatt
Copy link
Contributor

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

@jtimon
Copy link
Contributor Author

jtimon commented Jan 26, 2017

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.

@theuni
Copy link
Member

theuni commented Jan 26, 2017

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

@gmaxwell
Copy link
Contributor

Does this break being able to concatenate multiple transactions to merge signatures?

@laanwj
Copy link
Member

laanwj commented Jan 26, 2017

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?

@sipa
Copy link
Member

sipa commented Jan 26, 2017

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

@laanwj
Copy link
Member

laanwj commented Jan 26, 2017

BTW: this needs a test

@TheBlueMatt
Copy link
Contributor

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.

@laanwj laanwj added this to the 0.14.0 milestone Feb 2, 2017
@TheBlueMatt
Copy link
Contributor

Added a test for this in #9650.

@jtimon
Copy link
Contributor Author

jtimon commented Feb 2, 2017

As this is included in #9650 plus tests, and it is still a small PR, I'm closing this.

@jtimon jtimon closed this Feb 2, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants