Refactor analyzepsbt for use outside RPC code

gwillen commented at 8:35 am on March 1, 2019: contributor

Refactor the analyzepsbt RPC into (1) an AnalyzePSBT function, which returns its output as a new strongly-typed PSBTAnalysis struct, and (2) a thin wrapper which converts the struct into a UniValue for RPC use.

As with my previous refactoring PR, I need this because I am creating a dependency on this code from the GUI. Per discussion in #bitcoin-core-dev on IRC, since we don’t want to create a dependency on UniValue in anything outside RPC, I introduced some new structs to hold the info we get when analyzing a PSBT. For the field types, I used whatever types are already used internally for this data (e.g. CAmount, CFeeRate, CKeyID), and only convert to int/string etc. in the wrapper. @achow101, maybe take the first look? :-)

fanquake added the label RPC/REST/ZMQ on Mar 1, 2019

fanquake added the label Wallet on Mar 1, 2019

fanquake commented at 8:42 am on March 1, 2019: member

Travis:

0A new circular dependency in the form of "core_io -> psbt -> core_io" appears to have been introduced.
1^---- failure generated from test/lint/lint-circular-dependencies.sh

gwillen commented at 7:50 pm on March 1, 2019: contributor

Hmm, that’s not great, is it. I guess I forgot to run lint-all, sorry. (I have to raise the file descriptor limit before running, or it crashes on my mac, so it’s easy to forget…)

I am a little confused about this lint, because it appears I didn’t introduce a circular header dependency – rather, core_io.h (the interface) depends on psbt.h, but psbt.cpp (the implementation) depends on core_io.h. This doesn’t seem especially offensive to me, since it’s not actually circular? Is it intentional that the lint flags this situation?

gwillen force-pushed on Mar 1, 2019

gwillen commented at 7:54 pm on March 1, 2019: contributor

… but apparently the dependency was no longer needed anyway, so I don’t need to litigate this. ;-) Fixed!

in src/psbt.cpp:427 in 3a426fd7ce outdated

421+        CCoinsView view_dummy;
422+        CCoinsViewCache view(&view_dummy);
423+        bool success = true;
424+
425+        for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
426+            PSBTInput& input = psbtx.inputs[i];

practicalswift commented at 4:26 pm on March 2, 2019:

The scope of input could be reduced here without any change in behaviour, right?

gwillen commented at 10:42 pm on March 2, 2019:

Yes, that’s true – would you prefer I did? I normally agree about “less scope is better”, but here that instinct is clashing with my instinct to keep parallel things together, and definitions at the top of the relevant block.

practicalswift commented at 9:28 am on March 3, 2019:

Thanks for the explanation. Makes sense!

in src/psbt.h:625 in 3a426fd7ce outdated

614+ * Provides helpful miscellaneous information about where a PSBT is in the signing workflow.
615+ *
616+ * @param[in] psbtx the PSBT to analyze
617+ * @return A PSBTAnalysis with information about the provided PSBT.
618+ */
619+PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx);

promag commented at 4:10 pm on March 3, 2019:

const PartiallySignedTransaction& psbtx?

gwillen commented at 8:45 am on March 4, 2019:

I tried, but I had to back it out, because of the pattern of using “SignPSBTInput(DUMMY_SIGNING_PROVIDER, …)” to get signature data without modifying the PSBT. So we either have to take the psbt by value, or by non-const reference.

We could get around this with a const cast, but I’m against it unless we think the cost of copying a PartiallySignedTransaction is truly unacceptable.

promag commented at 3:45 pm on March 7, 2019:

Makes sense to keep by value. The code could be refactored but at the end each input would have to be copied to be able to call FillSignatureData() and FromSignatureData.

in src/psbt.cpp:434 in 3a426fd7ce outdated

423+        bool success = true;
424+
425+        for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
426+            PSBTInput& input = psbtx.inputs[i];
427+            if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, 1, nullptr, true)) {
428+                mtx.vin[i].scriptSig = input.final_script_sig;

promag commented at 4:12 pm on March 3, 2019:

Could move this after GetInputUTXO()?

gwillen commented at 3:18 am on March 6, 2019:

I’d be happy to, but I’m not sure I understand why? Can you elaborate?

promag commented at 1:24 am on March 7, 2019:

consider nit, just one loop exit:

0if (!SignPSBTInput(...) || !psbtx.GetInputUTXO(...)) {
1     success = false
2     break;
3}

gwillen commented at 11:07 pm on March 12, 2019:

Done.

promag commented at 0:03 am on March 13, 2019:

nit, could drop else { block or invert conditions:

 0for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
 1    PSBTInput& input = psbtx.inputs[i];
 2    Coin newcoin;
 3
 4    if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, 1, nullptr, true) && psbtx.GetInputUTXO(newcoin.out, i)) {
 5        mtx.vin[i].scriptSig = input.final_script_sig;
 6        mtx.vin[i].scriptWitness = input.final_script_witness;
 7        newcoin.nHeight = 1;
 8        view.AddCoin(psbtx.tx->vin[i].prevout, std::move(newcoin), true);
 9    } else {
10        success = false;
11        break;
12    }
13}

gwillen commented at 0:13 am on March 13, 2019:

At this point I think I better leave it as Future Work.

in src/psbt.cpp:336 in 3a426fd7ce outdated

329@@ -325,3 +330,139 @@ TransactionError CombinePSBTs(PartiallySignedTransaction& out, const std::vector
330 
331     return TransactionError::OK;
332 }
333+
334+std::string PSBTRoleName(PSBTRole role) {
335+    switch (role) {
336+        case PSBTRole::UPDATER:   return "updater";

promag commented at 4:15 pm on March 3, 2019:

I think we align case with switch. nit, I’d ignore returns alignment.

gwillen commented at 3:36 am on March 6, 2019:

Done.

gwillen commented at 8:48 am on March 4, 2019: contributor

Hmm, I seem to have introduced a linker problem that does not happen on OS X. I will get around to looking at it in a Linux VM, but if someone could look at it and go “oh, I know what you did wrong”, I’m sure it would save me a bunch of time. ;-)

sipa commented at 8:58 am on March 4, 2019: member

psbt.cpp is in libbitcoin_common, and consensus/tx_verify.cpp is in libbitcoin_server. libbitcoin_common in linked into bitcoin-tx, but libbitcoin_server isn’t.

gwillen commented at 3:11 am on March 6, 2019: contributor

Thanks @sipa.

The easiest solution seems – after whipping out a linux machine and doing a lot of repeatedly fiddling with Makefile.am and rebuilding everything from scratch – to be moving psbt.cpp from libbitcoin_common to libbitcoin_server. LMK what you think of that plan.

Unfortunately the tree of dependencies from AnalyzePSBT into libbitcoin_server runs very deep. The chain of dependencies is mostly fake – of the form “X coincidentally exists in the same file as something that depends on Y” – but still seems annoying to break.

The actual issue is not with most of the functionality of AnalyzePSBT, but just with the ancillary bit at the end which computes the size of a transaction, in order to compute the feerate. So one other option would be to break that bit out into a separate function and throw it into libbitcoin_server somewhere.

gwillen force-pushed on Mar 6, 2019

DrahtBot commented at 7:10 am on March 9, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15638 (Move-only: Pull wallet code out of libbitcoin_server by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Mar 12, 2019

in src/psbt.h:563 in 5f96bf2922 outdated

555+    SIGNER,
556+    FINALIZER,
557+    EXTRACTOR
558+};
559+
560+struct PSBTInputAnalysis {

ryanofsky commented at 7:15 pm on March 12, 2019:

It’d be nice to have some documentation for these structs. There could be a comment saying struct fields correspond to fields returned by the analyzepsbt method, if you didn’t want to just copy the documentation from analyzepsbt

gwillen commented at 11:07 pm on March 12, 2019:

Nah, thanks, this is a good point, I may as well document them properly.

ryanofsky approved

ryanofsky commented at 7:18 pm on March 12, 2019: member

utACK 5f96bf29225d7943314d68e10e19ee2fe3ccf3f3. I left a suggestion, but think this looks good as is. I just compared the moved code to make sure behavior did not appear to be changing.

gwillen force-pushed on Mar 12, 2019

gwillen commented at 11:52 pm on March 12, 2019: contributor

Ok, added documentation, and did the refactor requested by @promag. Sorry @ryanofsky, if you wouldn’t mind glancing at the minor code change and re-acking. I am convinced it does not change behavior.

Oh, and rebased – I picked up @sipa ’s fix from #15582 .

DrahtBot removed the label Needs rebase on Mar 12, 2019

gwillen commented at 0:14 am on March 13, 2019: contributor

Travis failure is weird:

00.20s$ curl -sSf -o python-3.5.tar.bz2 ${archive_url}
1curl: (22) The requested URL returned error: 403 Forbidden
2Unable to download 3.5 archive. The archive may not exist. Please consider a different version.

sipa commented at 0:21 am on March 13, 2019: member

It’s really unfortunate that the PSBT code needs to be in libbitcoin_server. I’m thinking of creating a separate PSBT signer tool using the signer/psbt source code, but with it in libbitcoin_server, that tool will need to include the whole networking, RPC, and validation logic.

Is it depending on anything more than GetVirtualTransactionSize? If not, I would suggest just moving GetTransactionWeight and friends from src/consensus/validation.h, and GetVirtualTransactionSize and friends from src/policy/policy.{h,cpp} to src/primitives/{transaction,block} respectively.

gwillen commented at 1:39 am on March 13, 2019: contributor

@sipa I’m happy to give it a shot.

From my notes, there were two offenders:

GetVirtualTransactionSize, as noted, which lives in policy.cpp
GetTransactionSigOpCost, which lives in consensus/tx_verify.cpp

The former seems easy to deal with – it’s just a small amount of math involving a couple of constants.

The latter might also be relatively standalone – the file it’s in has deeper dependencies spidering into libbitcoin_server, but I couldn’t tell if they were real dependencies of GetTransactionSigOpCost or if they were coincidentally being dragged in by something else.

Do you have a proposed place to move GetTransactionSigOpCost?

gwillen commented at 1:39 am on March 13, 2019: contributor

(The other option would be to remove the size and feerate calculations from AnalyzePSBT, or move just those elsewhere. They’re pretty standalone.)

sipa commented at 1:52 am on March 13, 2019: member

I have a simpler solution: replace the GetTransactionSigOpCost() call with 0 (which means the whole call can just become GetVirtualTransactionSize(ctx), and only that function needs to move elsewhere).

To explain why this is safe: reason why GetVirtualTransactionSize takes in a sigop count as argument is because in the mempool/feerate/mining code we use vsize = max(actual_vsize(tx), sigops(tx)*someconstant). This brings everything to a single unit we can optimize for, but still guarantees that transactions with excessively high amount of signature validation operations won’t cause the block-wide sigops limit to be exceeded.

However, the wallet/signing/psbt side of things doesn’t need to care about this. They will never deal with transactions where the sigop side of that max dominates (it would require more signature checks than 1 per 20 bytes of witness data, implying that every signature is verified on average more than 3 times). Such transactions are almost certainly pathologically constructed, and certainly not something SignPSBTInput could deal with anyway.

EDIT: This also lets you drop the whole CCoinsViewCache view object and construction thereof.

DrahtBot added the label Needs rebase on Mar 13, 2019

ryanofsky approved

ryanofsky commented at 3:19 pm on March 13, 2019: member

utACK 9d98086f22733cd8363d3b1df022d6e7f7dfd626. Changes since last review: changing if/else order not assigning empty fields, adding documentation as suggested in previous comments, also changing int to CAmount.

gwillen force-pushed on Mar 14, 2019

gwillen commented at 0:29 am on March 14, 2019: contributor

ok, my apologies to @ryanofsky and I appreciate your patience, this version has exactly two changes:

Rebase over #15559, which changes the format of the estimated feerate from a string to an integer in the RPC output (does not affect the output of the AnalyzePSBT function itself, only how it’s formatted in the wrapper in rpc/rawtransaction.cpp)
Per @sipa ’s request, revert the move into libbitcoin_server, putting psbt.cpp back in libbitcoin_common. Fingers crossed that Travis likes this, but it appears experimentally that moving the PSBT-related functions from core_io into psbt is enough to satisfy the linker, without actually moving PSBT around.

(This does not quite actually solve his problem; the reason the linker accepts this is that bitcoin-tx has no dependency on psbt.cpp, and the corresponding object file is now being dropped from the link, so the fact that it has in turn a broken dependency never comes up. Actually using psbt without linking bitcoin_server will still give a link error. However, it will be much easier to fix this once the code that is using psbt actually exists, because right now it’s very hard to get the linker not to optimize out the problem, so it’s really hard to confidently fix it.)

DrahtBot removed the label Needs rebase on Mar 14, 2019

ryanofsky approved

ryanofsky commented at 8:42 pm on March 14, 2019: member

utACK deef14c4f5d6c997e5cc68fc4007104dc2051606. Only changes since last review are leaving psbt.cpp in libbitcoin_common (instead of libbitcoin_server) as described above and rebasing after conflict with #15559

gwillen commented at 1:41 am on March 20, 2019: contributor

Definitely hoping to get this merged soon. @sipa or @promag, maybe take a look and see if you have anything further? :-)

in src/core_read.cpp:180 in 078994ad4a outdated

176@@ -177,33 +177,6 @@ bool DecodeHexBlk(CBlock& block, const std::string& strHexBlk)
177     return true;
178 }
179 
180-bool DecodeBase64PSBT(PartiallySignedTransaction& psbt, const std::string& base64_tx, std::string& error)

sipa commented at 10:24 pm on March 26, 2019:

I think this means you can remove the #include <psbt.h> from this file.

gwillen commented at 0:46 am on March 27, 2019:

yep, done

in src/core_io.h:40 in 078994ad4a outdated

36@@ -37,11 +37,6 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string& hex_header);
37  */
38 bool ParseHashStr(const std::string& strHex, uint256& result);
39 std::vector<unsigned char> ParseHexUV(const UniValue& v, const std::string& strName);
40-

sipa commented at 10:25 pm on March 26, 2019:

I think this means you can remove the struct PartiallySignedBitcoinTransaction forward declare from this file.

gwillen commented at 0:46 am on March 27, 2019:

yep, done

in src/rpc/rawtransaction.cpp:1902 in e01983a9f0 outdated

1905-    bool only_missing_sigs = true;
1906-    bool only_missing_final = false;
1907-    CAmount in_amt = 0;
1908-    for (unsigned int i = 0; i < psbtx.tx->vin.size(); ++i) {
1909-        PSBTInput& input = psbtx.inputs[i];
1910+    for (auto input : psbta.inputs) {

sipa commented at 10:26 pm on March 26, 2019:

const auto& input to avoid copies.

gwillen commented at 0:46 am on March 27, 2019:

done

in src/rpc/rawtransaction.cpp:1938 in e01983a9f0 outdated

2004 
2005-    if (all_final) {
2006-        only_missing_sigs = false;
2007-        result.pushKV("next", "extractor");
2008+    if (psbta.estimated_vsize != nullopt) {
2009+        result.pushKV("estimated_vsize", (int)*psbta.estimated_vsize);

sipa commented at 10:30 pm on March 26, 2019:

Is this int cast necessary?

gwillen commented at 0:33 am on March 27, 2019:

According to the compiler, some cast is required:

 0rpc/rawtransaction.cpp:1938:16: error: call to member function 'pushKV' is ambiguous
 1        result.pushKV("estimated_vsize", *psbta.estimated_vsize);
 2        ~~~~~~~^~~~~~
 3./univalue/include/univalue.h:124:10: note: candidate function
 4    bool pushKV(const std::string& key, int64_t val_) {
 5         ^
 6./univalue/include/univalue.h:128:10: note: candidate function
 7    bool pushKV(const std::string& key, uint64_t val_) {
 8         ^
 9./univalue/include/univalue.h:132:10: note: candidate function
10    bool pushKV(const std::string& key, bool val_) {
11         ^
12./univalue/include/univalue.h:136:10: note: candidate function
13    bool pushKV(const std::string& key, int val_) {
14         ^
15./univalue/include/univalue.h:140:10: note: candidate function
16    bool pushKV(const std::string& key, double val_) {
17         ^
18./univalue/include/univalue.h:115:10: note: candidate function
19    bool pushKV(const std::string& key, const UniValue& val);

sipa commented at 0:34 am on March 28, 2019:

Looks like it’s needed in that case.

sipa commented at 10:31 pm on March 26, 2019: member

utACK deef14c4f5d6c997e5cc68fc4007104dc2051606, only nits

Move PSBT decoding functions from core_io to psbt.cpp

Move PSBT decoding functions from core_io.h/core_read.cpp to psbt.h/psbt.cpp,
to deal with a linker issue.

afd20a25f2

Refactor analyzepsbt for use outside RPC code

Refactor the analyzepsbt RPC into (1) an AnalyzePSBT function, which returns
its output as a new strongly-typed PSBTAnalysis struct, and (2) a thin wrapper
which converts the struct into a UniValue for RPC use.

ef22fe8c1f

Add documentation of struct PSBTAnalysis et al 892eff05f1

gwillen force-pushed on Mar 27, 2019

gwillen commented at 0:46 am on March 27, 2019: contributor

@sipa Thanks, all your comments addressed.

sipa commented at 1:27 am on March 28, 2019: member

utACK 892eff05f115c0b002d0e0b6ffc3ab418480d25c

sipa commented at 10:29 pm on March 28, 2019: member

Feel like reviewing, @achow101?

ryanofsky approved

ryanofsky commented at 4:33 pm on March 29, 2019: member

utACK 892eff05f115c0b002d0e0b6ffc3ab418480d25c. Just small cleanups since the last review: removing unneeded include, forward decl, adding const ref

achow101 commented at 10:00 pm on April 2, 2019: member

utACK 892eff05f115c0b002d0e0b6ffc3ab418480d25c

sipa merged this on Apr 6, 2019

sipa closed this on Apr 6, 2019

sipa referenced this in commit e439aeb30c on Apr 6, 2019

gwillen deleted the branch on Apr 8, 2019

deadalnix referenced this in commit 1daa2cf98c on May 16, 2020

deadalnix referenced this in commit 27384f6968 on May 16, 2020

deadalnix referenced this in commit aa96a2979b on May 16, 2020

Munkybooty referenced this in commit a981f739c8 on Sep 30, 2021

DrahtBot locked this on Dec 16, 2021

Refactor analyzepsbt for use outside RPC code #15508

Conflicts