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.

Sign up for GitHub

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

Jump to bottom

rpc: Properly deserialize txs with witness before signing #19836

Merged
merged 2 commits into from
Oct 16, 2020
Merged

rpc: Properly deserialize txs with witness before signing #19836

merged 2 commits into from
Oct 16, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 30, 2020

Signing a transaction can only happen when the transaction has inputs. A transaction with inputs can always be deserialized as witness-transaction. If try_no_witness decoding is attempted, this will lead to rare intermittent failures.

Fixes #18803

@maflcko
Copy link
Member Author

maflcko commented Aug 30, 2020

This may have been introduced in commit 6b4f231, in which case it needs backport to all currently supported, released versions of Bitcoin Core.

@maflcko
Copy link
Member Author

maflcko commented Aug 30, 2020

See also #15899

@fanquake fanquake requested a review from achow101 August 31, 2020 04:48
@laanwj
Copy link
Member

laanwj commented Sep 2, 2020

Looks like this removes all the remaining DecodeHexTx(…, …, true) in non-test code, besides fundrawtransaction.

@achow101
Copy link
Member

achow101 commented Sep 2, 2020

What happens if someone does try a 0 input tx with these? Ideally we would give a meaningful error, but I don't think we do.

@fanquake
Copy link
Member

@MarcoFalke could you follow up with achows query:

What happens if someone does try a 0 input tx with these? Ideally we would give a meaningful error, but I don't think we do.

@maflcko
Copy link
Member Author

maflcko commented Oct 13, 2020

@achow101 @fanquake thanks, adjusted error message

@maflcko maflcko added this to the 0.21.0 milestone Oct 13, 2020
achow101
achow101 reviewed Oct 13, 2020
View reviewed changes
Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3333077

@maflcko
Copy link
Member Author

maflcko commented Oct 14, 2020

@instagibbs / @ajtowns as the author/reviewer of #17775 you might be qualified and interested in reviewing this

@ajtowns
Copy link
Contributor

ajtowns commented Oct 15, 2020

ACK 3333077

Confirmed the RPCs changed are only worth calling if you have inputs, and the error message changes look okay.

@fanquake fanquake merged commit cbb5f3a into bitcoin:master Oct 16, 2020
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 16, 2020
@fanquake
rpc: Properly deserialize txs with witness before signing
731502a 
Github-Pull: bitcoin#19836
Rebased-From: cccc752
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 16, 2020
@fanquake
rpc: Adjust witness-tx deserialize error message
bdf15d0 
Github-Pull: bitcoin#19836
Rebased-From: 3333077
@maflcko maflcko deleted the 2008-rpcDeserTxsWitness branch October 16, 2020 04:19
@fanquake fanquake mentioned this pull request Oct 16, 2020
Merged
@fanquake
Copy link
Member

Being backported to 0.20 in #20166.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 16, 2020
@fanquake
rpc: Properly deserialize txs with witness before signing
a7bdf5c 
Github-Pull: bitcoin#19836
Rebased-From: cccc752
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Oct 16, 2020
@fanquake
rpc: Adjust witness-tx deserialize error message
9c71499 
Github-Pull: bitcoin#19836
Rebased-From: 3333077
@fanquake fanquake mentioned this pull request Oct 16, 2020
Merged
@fanquake
Copy link
Member

Added to #20150 for 0.19 backporting.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 16, 2020
@fanquake
Merge bitcoin#19836: rpc: Properly deserialize txs with witness befor…
e0ffdf5 
…e signing

3333077 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
cccc752 rpc: Properly deserialize txs with witness before signing (MarcoFalke)

Pull request description:

  Signing a transaction can only happen when the transaction has inputs. A transaction with inputs can always be deserialized as witness-transaction. If `try_no_witness` decoding is attempted, this will lead to rare intermittent failures.

  Fixes bitcoin#18803

ACKs for top commit:
  achow101:
    ACK 3333077
  ajtowns:
    ACK 3333077

Tree-SHA512: 73f8a5cdfe03fb0e68908d2fa09752c346406f455694a020ec0dd1267ef8f0a583b8e84063ea74aac127106dd193b72623ca6d81469a94b3f5b3c766ebf2c42b
maflcko pushed a commit that referenced this pull request Nov 18, 2020
Merge #20166: [0.20] Backports
a2fa11f 
7566af4 doc: Update data directory path comments (Hennadii Stepanov)
09261de util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
8ef0dac macOS deploy: use the new plistlib API (Jonas Schnelli)
314e795 build: fix mutex detection when building bdb on macOS (fanquake)
1f67a30 random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman)
6113b54 net: Send post-verack handshake messages at most once (MarcoFalke)
bdf15d0 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
731502a rpc: Properly deserialize txs with witness before signing (MarcoFalke)
ee0082b Avoid the use of abs64 in timedata (Pieter Wuille)
05bd0c2 docs: Correct description for getblockstats's txs field (Nadav Ivgi)

Pull request description:

  Backports the following PRs to the 0.20 branch:
  * #19777 - docs: Correct description for getblockstats's txs field
  * #19836 - rpc: Properly deserialize txs with witness before signing
  * #20080 - Strip any trailing `/` in -datadir and -blocksdir paths
  * #20082 - [bugfix] random: fixes read buffer to use min rather than max
  * #20141 - Avoid the use of abs64 in timedata
  * #20146 - net: Send post-verack handshake messages at most once
  * #20195 - build: fix mutex detection when building bdb on macOS
  * #20298 - macOS deploy: use the new plistlib API

  Will add additional commits as they become available.

ACKs for top commit:
  MarcoFalke:
    review ACK 7566af4 🗡

Tree-SHA512: add6bb978313c12c3e07bc232636ae9d1ab0edd0b816705c5c70eeb1cc04097165fd5e29d60c706886943ceb1f749a422020766b4aa2d23be51e9f839157a4bb
MarkLTZ pushed a commit to MarkLTZ/bitcoin that referenced this pull request Nov 21, 2020
@MarkLTZ
rpc: Properly deserialize txs with witness before signing
2b539ee 
Github-Pull: bitcoin#19836
Rebased-From: cccc752
MarkLTZ pushed a commit to MarkLTZ/bitcoin that referenced this pull request Nov 21, 2020
@MarkLTZ
rpc: Adjust witness-tx deserialize error message
e386ce4 
Github-Pull: bitcoin#19836
Rebased-From: 3333077
MarkLTZ pushed a commit to MarkLTZ/bitcoin that referenced this pull request Nov 21, 2020
@MarkLTZ
Merge bitcoin#20166: [0.20] Backports
c61e643 
7566af4 doc: Update data directory path comments (Hennadii Stepanov)
09261de util: Add StripRedundantLastElementsOfPath function (Hennadii Stepanov)
8ef0dac macOS deploy: use the new plistlib API (Jonas Schnelli)
314e795 build: fix mutex detection when building bdb on macOS (fanquake)
1f67a30 random: fixes read buffer resizing in RandAddSeedPerfmon (Ethan Heilman)
6113b54 net: Send post-verack handshake messages at most once (MarcoFalke)
bdf15d0 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
731502a rpc: Properly deserialize txs with witness before signing (MarcoFalke)
ee0082b Avoid the use of abs64 in timedata (Pieter Wuille)
05bd0c2 docs: Correct description for getblockstats's txs field (Nadav Ivgi)

Pull request description:

  Backports the following PRs to the 0.20 branch:
  * bitcoin#19777 - docs: Correct description for getblockstats's txs field
  * bitcoin#19836 - rpc: Properly deserialize txs with witness before signing
  * bitcoin#20080 - Strip any trailing `/` in -datadir and -blocksdir paths
  * bitcoin#20082 - [bugfix] random: fixes read buffer to use min rather than max
  * bitcoin#20141 - Avoid the use of abs64 in timedata
  * bitcoin#20146 - net: Send post-verack handshake messages at most once
  * bitcoin#20195 - build: fix mutex detection when building bdb on macOS
  * bitcoin#20298 - macOS deploy: use the new plistlib API

  Will add additional commits as they become available.

ACKs for top commit:
  MarcoFalke:
    review ACK 7566af4 🗡

Tree-SHA512: add6bb978313c12c3e07bc232636ae9d1ab0edd0b816705c5c70eeb1cc04097165fd5e29d60c706886943ceb1f749a422020766b4aa2d23be51e9f839157a4bb
maflcko pushed a commit that referenced this pull request Dec 2, 2020
Merge #20150: [0.19] Backports
5baaea4 
9c71499 rpc: Adjust witness-tx deserialize error message (MarcoFalke)
a7bdf5c rpc: Properly deserialize txs with witness before signing (MarcoFalke)
0b64310 Avoid the use of abs64 in timedata (Pieter Wuille)
5b2de04 Bump vcpkg commit ID to get new msys mirror list (Aaron Clauson)
6957419 build: set minimum required Boost to 1.48.0 (fanquake)
27bb2cc util: Don't reference errno when pthread fails. (MIZUTA Takeshi)
8bd2ab1 docs: Correct description for getblockstats's txs field (Nadav Ivgi)
a8411b3 qt: Fix QFileDialog for static builds (Hennadii Stepanov)

Pull request description:

  Backports the following to the 0.19 branch:
  * #19194 - util: Don't reference errno when pthread fails. - not clean.
  * #19536 - qt, build: Fix QFileDialog for static builds
  * #19777 - docs: Correct description for getblockstats's txs field
  * #19836 - rpc: Properly deserialize txs with witness before signing
  * #20095 - CI: Bump vcpkg commit ID to get new msys mirror list
  * #20141 - Avoid the use of abs64 in timedata
  * #20142 - [0.20] build: set minimum required Boost to 1.48.0

ACKs for top commit:
  jnewbery:
    utACK 9c71499
  dergoegge:
    utACK 9c71499
  MarcoFalke:
    ACK 9c71499

Tree-SHA512: 2151f22bc37a6a2f51a8f36c27376622016b51ff99b570e95354356fce1f1761cf19cb4f8ebfa26d38485a0bff6ff6ee834d2798fb383e2ae2abb175548b8fe6
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achow101 achow101 achow101 left review comments

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
0.21.0
5 participants
@maflcko @laanwj @achow101 @fanquake @ajtowns