Paths

Table of Contentst

Differential D5715

Refactor PSBT signing logic to enforce invariant
ClosedPublic
Actions

Authored by Fabien on Apr 13 2020, 19:03.

Tags

None

Subscribers

Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGINGf55f0c3e08ae: Refactor PSBT signing logic to enforce invariant
rABCf55f0c3e08ae: Refactor PSBT signing logic to enforce invariant

Summary

Backport of core PR14588.

This refactor repairs the invariant, [...]. It also simplifies some
other code, and removes redundant parameters from some related
functions.

Note that the bug fixed in the PR is not affecting us, but I kept the
test as it can't hurt.

Also the IsSane() method is kept for easier backports, despite it does
nothing and will be optimized out by the compiler.

Test Plan

ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Apr 13 2020, 19:03

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 13 2020, 19:03

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10198: Diff 18774.Apr 13 2020, 19:15

Fabien added a child revision: D5718: Move PSBT definitions and code to separate files.Apr 13 2020, 20:08

deadalnix requested changes to this revision.Apr 13 2020, 22:42

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/wallet/rpcwallet.cpp
4893 ↗	(On Diff #18774)	Why did you change the name? There is only downsides afaik. Please revert to `rawTx`.

This revision now requires changes to proceed.Apr 13 2020, 22:42

Fabien planned changes to this revision.Apr 14 2020, 08:50

Fabien added inline comments.

src/wallet/rpcwallet.cpp
4893 ↗	(On Diff #18774)	Since I need to convert the CMutableTransaction to a CTransaction (core does implicit conversion) I thought mtx vs tx was better than rawTx vs tx. Not a strong opinion though, so I will revert to avoid confusion.

mTx => rawTx

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10213: Diff 18793.Apr 14 2020, 09:04

deadalnix accepted this revision.Apr 14 2020, 13:40

deadalnix added inline comments.

src/wallet/rpcwallet.cpp
4899 ↗	(On Diff #18793)	Why create a temporary? You can simply do PartiallySignedTransaction psbtx(CTransaction(rawTx));

This revision is now accepted and ready to land.Apr 14 2020, 13:40

Fabien added inline comments.Apr 15 2020, 10:51

src/wallet/rpcwallet.cpp
4899 ↗	(On Diff #18793)	That won't build, at least with clang 9.

Closed by commit rABCf55f0c3e08ae: Refactor PSBT signing logic to enforce invariant (authored by Glenn Willen <gwillen@nerdnet.org>, committed by Fabien). · Explain WhyApr 15 2020, 10:51

This revision was automatically updated to reflect the committed changes.

Fabien added a commit: rABCf55f0c3e08ae: Refactor PSBT signing logic to enforce invariant.

Fabien added a commit: rSTAGINGf55f0c3e08ae: Refactor PSBT signing logic to enforce invariant.May 5 2020, 18:57

nakihito mentioned this in D6073: [backport#14906] refactor: Make explicit CMutableTransaction -> CTransaction conversion..May 14 2020, 20:24

Revision Contents
Changeset List

Path

Size

src/

rpc/

rawtransaction.cpp

17 lines

script/

6 lines

24 lines

wallet/

1 line

70 lines

test/

psbt_wallet_tests.cpp

6 lines

test/

functional/

13 lines

Diff 18821

src/rpc/rawtransaction.cpp

Loading...

src/script/sign.h

Loading...

src/script/sign.cpp

Loading...

src/wallet/rpcwallet.h

Loading...

src/wallet/rpcwallet.cpp

Loading...

src/wallet/test/psbt_wallet_tests.cpp

Loading...

test/functional/rpc_psbt.py

Loading...