BIP-322 support #20154

kallewoof commented at 12:48 PM on October 15, 2020: member

This is my WIP on BIP 322 support. It is based on the updated BIP which significantly changes how message signing is done.

add framework for transaction proofs (BIP-322)

Initially the proofs are put into the signet.h/cpp files as they are reusing a lot of the functionality.

35bdd745a4

wip provefundswithwallet, verifymessage, ... dd69e9e5e2

DrahtBot added the label GUI on Oct 15, 2020

DrahtBot added the label RPC/REST/ZMQ on Oct 15, 2020

DrahtBot added the label Utils/log/libs on Oct 15, 2020

DrahtBot added the label Wallet on Oct 15, 2020

DrahtBot commented at 4:26 PM on October 15, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20480 (Replace boost::variant with std::variant by MarcoFalke)
#20012 (rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke)
#19183 ([WIP DONOTMERGE] Replace boost with C++17 by MarcoFalke)
#18466 (rpc: fix invalid parameter error codes for {sign,verify}message RPCs by theStack)
#15129 (rpc: Added ability to remove watch only addresses by benthecarman)

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 cross-referenced this on Oct 15, 2020 from issue rpc: fix invalid parameter error codes for {sign,verify}message RPCs by theStack

DrahtBot cross-referenced this on Oct 16, 2020 from issue rpc: Added ability to remove watch only addresses by benthecarman

use correct amounts in sig checks 6ccb0c1e74

bip322: check that all given inputs are included in proof 550aa48a39

handle lock time 03506dc9e4

in src/qt/signverifymessagedialog.cpp:251 in 03506dc9e4

 245 | @@ -246,6 +246,21 @@ void SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
 246 |              QString("<nobr>") + tr("Message verification failed.") + QString("</nobr>")
 247 |          );
 248 |          return;
 249 | +    case MessageVerificationResult::INCONCLUSIVE:
 250 | +        ui->statusLabel_VM->setText(
 251 | +            tr("Message inconclusive (your node may need to be upgraded to validate new upgradable rules).")

luke-jr commented at 7:00 PM on October 24, 2020:

This might need rephrasing to avoid an attack where someone uses this to trick a user into "upgrading" to malware...

in src/qt/signverifymessagedialog.cpp:261 in 03506dc9e4

 256 | +            tr("Message inconclusive, and encumbered by an unexpired time lock (your node may need to be upgraded to validate new upgradable rules).")
 257 | +        );
 258 | +        return;
 259 | +    case MessageVerificationResult::VALID_IN_FUTURE:
 260 | +        ui->statusLabel_VM->setText(
 261 | +            tr("Message valid, but encumbered by an unexpired time lock.")

luke-jr commented at 7:01 PM on October 24, 2020:

This is probably confusing to end users.

in src/rpc/misc.cpp:280 in 03506dc9e4

 276 | @@ -276,6 +277,16 @@ static RPCHelpMan verifymessage()
 277 |                      {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
 278 |                      {"signature", RPCArg::Type::STR, RPCArg::Optional::NO, "The signature provided by the signer in base 64 encoding (see signmessage)."},
 279 |                      {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message that was signed."},
 280 | +                    {"inputs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "UTXOs in a proof of funds",

luke-jr commented at 7:03 PM on October 24, 2020:

This should probably be an alternative to position 1 ("address"), and support a simpler "amount" usage.

in src/wallet/rpcwallet.cpp:3373 in 03506dc9e4

3368 | +    return RPCHelpMan{"provefundswithwallet",
3369 | +        "\nProve ownership of an optional address and an optional array of UTXOs.\n" +
3370 | +        HELP_REQUIRING_PASSPHRASE,
3371 | +        {
3372 | +            {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message that should be embedded in the proof."},
3373 | +            {"address", RPCArg::Type::STR, "", "Address to prove (should be omitted when including inputs)"},

luke-jr commented at 7:04 PM on October 24, 2020:

Funds don't have addresses. This makes no sense.

in src/wallet/rpcwallet.cpp:3374 in 03506dc9e4

3369 | +        "\nProve ownership of an optional address and an optional array of UTXOs.\n" +
3370 | +        HELP_REQUIRING_PASSPHRASE,
3371 | +        {
3372 | +            {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message that should be embedded in the proof."},
3373 | +            {"address", RPCArg::Type::STR, "", "Address to prove (should be omitted when including inputs)"},
3374 | +            {"inputs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "UTXOs in a proof of funds",

luke-jr commented at 7:04 PM on October 24, 2020:

Should probably have a simpler amount-only alternative.

luke-jr changes_requested

luke-jr commented at 7:04 PM on October 24, 2020: member

Incomplete review.

DrahtBot cross-referenced this on Nov 19, 2020 from issue rpc: Remove duplicate name and argNames from CRPCCommand by MarcoFalke

DrahtBot cross-referenced this on Nov 19, 2020 from issue [WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) by MarcoFalke

DrahtBot cross-referenced this on Nov 24, 2020 from issue Replace boost::variant with std::variant by MarcoFalke

kallewoof commented at 6:17 AM on December 29, 2020: member

The BIP is changing substantially. Will revisit when that's done. @luke-jr Thanks for the feedback. Will take it into consideration.

kallewoof closed this on Dec 29, 2020

bitcoin locked this on Feb 15, 2022