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

[Wallet][RPC] FundTransaction - fundrawtransaction #1663

Merged
merged 24 commits into from
Jun 28, 2020

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Jun 4, 2020

based on top of

This introduces a new wallet function, CWallet::FundTransaction() (and exposes it via RPC with fundrawtransaction), to fill a tx containing only vouts (or not enough vins to cover the vouts) with unspent coins from the wallet.

fundrawtransaction will not modify existing inputs, and will add one change output (if needed) to the outputs. It will not sign the inputs (so can include also watch-only or multi-sig inputs, if enabled).

backported from:

adapting the tests for the (more recent) framework.

[*] Note: this has been included to be able to call fundrawtransaction without the need for an unencrypted wallet (for the change address key)

@random-zebra random-zebra added RPC Upstream Wallet Tests Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes labels Jun 4, 2020
@random-zebra random-zebra added this to the 4.2.0 milestone Jun 4, 2020
@random-zebra random-zebra self-assigned this Jun 4, 2020
@random-zebra random-zebra force-pushed the 202005_fundrawtransaction branch 2 times, most recently from 8771399 to ba6109b Compare June 5, 2020 20:45
@random-zebra
Copy link
Author

Rebased. Ready for review/QA.

@ambassador000
Copy link

fundrawtransaction RPC command is working as intended (also with encrypted wallet), however I couldn't get working createrawtransaction and signrawtransaction commands, always ended up with various errors, probably because of the confusing input format needed.

@random-zebra
Copy link
Author

random-zebra commented Jun 26, 2020

@NoobieDev12 , what kind of errors?
It should be straightforward.
Here's a brief tutorial to (1) create, (2) fund, (3) sign, and (4) broadcast a transaction:

  1. Create a transaction without inputs (empty list []), only outputs (dictionary {"address": amount}):
$ ./pivx-cli createrawtransaction '[]' '{"yCrjZ7EcS2y7jFo48dMvmUxPtp1fTt9W4U": 100}'

01000000000100e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000
  1. Fund the transaction (i.e. add enough inputs to cover the output values):
$ ./pivx-cli fundrawtransaction 01000000000100e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000

{
  "hex": "01000000014da53ec78886990bb07b27ab9ce67cc586be60c58cdc622bd35380657219a57f0000000000ffffffff02ace41b78030000001976a914d764b754e913a26bce160c6a5ca2c1c20ebb8dc588ac00e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000",
  "changepos": 0,
  "fee": 0.00002260
}
  1. The raw transaction ("hex" string from the output of the previous command) is unsigned, as we can see decoding it:
$ ./pivx-cli decoderawtransaction 01000000014da53ec78886990bb07b27ab9ce67cc586be60c58cdc622bd35380657219a57f0000000000ffffffff02ace41b78030000001976a914d764b754e913a26bce160c6a5ca2c1c20ebb8dc588ac00e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000

{
  "txid": "0d9ab9ea5324fc9d3d7ce98bf8b4e3bf722fc0404d105b42fd4f9b8ac7cbbcb0",
  "version": 1,
  "size": 119,
  "locktime": 0,
  "vin": [
    {
      "txid": "7fa51972658053d32b62dc8cc560be86c57ce69cab277bb00b998688c73ea54d",
      "vout": 0,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "sequence": 4294967295
    }
  ],
  ...
}

Let's add the signatures to the input:

$ ./pivx-cli signrawtransaction 01000000014da53ec78886990bb07b27ab9ce67cc586be60c58cdc622bd35380657219a57f0000000000ffffffff02ace41b78030000001976a914d764b754e913a26bce160c6a5ca2c1c20ebb8dc588ac00e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000

{
  "hex": "01000000014da53ec78886990bb07b27ab9ce67cc586be60c58cdc622bd35380657219a57f000000006a4730440220546266cdd03230f0c7c71aed643704651c50228c910b102c36d96780ec14c02f0220152ff071e9d67a836c7fbcc5c019e072692a836e8210830de17c5aa3501676aa012102271fb71ea03d9c9d6014d192dcda21de7d47f9044f6786b72761fb9c7034c162ffffffff02ace41b78030000001976a914d764b754e913a26bce160c6a5ca2c1c20ebb8dc588ac00e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000",
  "complete": true
}
  1. The transaction (again "hex" string from the output of the previous command) is now signed:
$ ./pivx-cli decoderawtransaction 01000000014da53ec78886990bb07b27ab9ce67cc586be60c58cdc622bd35380657219a57f000000006a4730440220546266cdd03230f0c7c71aed643704651c50228c910b102c36d96780ec14c02f0220152ff071e9d67a836c7fbcc5c019e072692a836e8210830de17c5aa3501676aa012102271fb71ea03d9c9d6014d192dcda21de7d47f9044f6786b72761fb9c7034c162ffffffff02ace41b78030000001976a914d764b754e913a26bce160c6a5ca2c1c20ebb8dc588ac00e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000

{
  "txid": "6751d397bc4e97bf444248844b783b83f652a4fb900d4df85fb148b9f89ee86d",
  "version": 1,
  "size": 225,
  "locktime": 0,
  "vin": [
    {
      "txid": "7fa51972658053d32b62dc8cc560be86c57ce69cab277bb00b998688c73ea54d",
      "vout": 0,
      "scriptSig": {
        "asm": "30440220546266cdd03230f0c7c71aed643704651c50228c910b102c36d96780ec14c02f0220152ff071e9d67a836c7fbcc5c019e072692a836e8210830de17c5aa3501676aa[ALL] 02271fb71ea03d9c9d6014d192dcda21de7d47f9044f6786b72761fb9c7034c162",
        "hex": "4730440220546266cdd03230f0c7c71aed643704651c50228c910b102c36d96780ec14c02f0220152ff071e9d67a836c7fbcc5c019e072692a836e8210830de17c5aa3501676aa012102271fb71ea03d9c9d6014d192dcda21de7d47f9044f6786b72761fb9c7034c162"
      },
      "sequence": 4294967295
    }
  ],
  ...
}

Let's broadcast it to the network:

$ ./pivx-cli sendrawtransaction 01000000014da53ec78886990bb07b27ab9ce67cc586be60c58cdc622bd35380657219a57f000000006a4730440220546266cdd03230f0c7c71aed643704651c50228c910b102c36d96780ec14c02f0220152ff071e9d67a836c7fbcc5c019e072692a836e8210830de17c5aa3501676aa012102271fb71ea03d9c9d6014d192dcda21de7d47f9044f6786b72761fb9c7034c162ffffffff02ace41b78030000001976a914d764b754e913a26bce160c6a5ca2c1c20ebb8dc588ac00e40b54020000001976a914ae29d55979ebcaceccf4d660810a3390e728b5fa88ac00000000

6751d397bc4e97bf444248844b783b83f652a4fb900d4df85fb148b9f89ee86d

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review ACK 5a41ebb.

src/wallet/wallet.cpp Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
furszy
furszy previously approved these changes Jun 27, 2020
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Working as intended 👌 , tested ACK 5a41ebb.

@ambassador000
Copy link

ambassador000 commented Jun 28, 2020

Thanks for the detailed reply and explanation RZ!
All working now! However, just for the reference, I cannot reproduce it again, but yesterday at some point using the fundrawtransaction I got the "changepos": -1, output. Other errors were mostly related to invalid inputs and are irrelevant because of that.

@Fuzzbawls
Copy link
Collaborator

Needs a rebase, but otherwise ACK

- backports bitcoin/bitcoin@1e0d1a2

Some code stolen from Jonas Schnelli <jonas.schnelli@include7.ch>
- backports bitcoin/bitcoin@0aad1f1

Since unspendable outputs can't be spent, there is no threshold at which
it would be uneconomic to spend them.

This primarily targets transaction outputs with `OP_RETURN`.
- backports bitcoin/bitcoin@d3354c5

This indicates that, eg, we have a public key for a key which may
be used as a pay-to-pubkey-hash. It generally means that we can
create a valid scriptSig except for missing private key(s) with
which to create signatures.

- backports bitcoin/bitcoin@428a898

[squash commit]
- backports bitcoin/bitcoin@6bdb474

Some code and test cases stolen from Bryan Bishop <bryan@ledgerx.com>
(pull bitcoin#5524).

- backports bitcoin/bitcoin@d042854

[squash commit]
- backports bitcoin/bitcoin@f7dc1d3

Strict flag forces type check on all object keys.
- backports bitcoin/bitcoin@db992ea

[PIVX: Don't keep backward compatibility (as this is introduced and
updated in the same PR)]
@random-zebra
Copy link
Author

Rebased, and added one last case to the functional test.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

re ACK fc81158 .

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK fc81158

perpetual updating PIVX Core to BTC Core automation moved this from In Progress to Ready Jun 28, 2020
@furszy furszy merged commit 0724bbb into PIVX-Project:master Jun 28, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done Jun 28, 2020
random-zebra added a commit that referenced this pull request Aug 5, 2020
d1d15c8 Fix missing sigverion in main_test.cpp CreateDummyScriptSigWithKey. (furszy)
a034daf Rename to PrecomputedTransactionData (furszy)
b4b181b Unit test for sighash caching (furszy)
2ef3872 Report non-mandatory script failures correctly. (furszy)
446d340 Precompute sighashes (furszy)
dfd24eb Update wallet_txn_close.py test: (furszy)
a5170f0 BIP143: Signing logic. (furszy)
d2dd547 BIP143: Verification logic. (furszy)
dccc3c6 Refactor script validation to observe amounts (furszy)
daf044a Reduce unnecessary hashing in signrawtransaction (furszy)

Pull request description:

  Base work for the new transaction digest algorithm for signature verification on PIVX Sapling transactions.

  Essentially, an implementation of BIP143 + few more good commits that found down the rabbit hole.

  Back ports:

  * bitcoin#7276
  * bitcoin#7976
  * bitcoin#8118
  * bitcoin#8149 (only amount validation and SignatureHash commits).
  * bitcoin#6088 (only the dummy signature one - will be removed once #1663 get merged -).
  * bitcoin#6379
  * bitcoin#8524

  Next step over this area (need 1553 merged to be able to push it) is the further specialization of BIP143 into our custom implementation of ZIP143 (with a different digest algorithm definition using our tx data and hash personalization).

ACKs for top commit:
  Fuzzbawls:
    utACK d1d15c8
  random-zebra:
    ACK d1d15c8 and merging...

Tree-SHA512: 7665cccf095c5bce0b18ef7ab8fcf7bede9304993b48f1af9c352c568861dec728d1d68671aab857b73d46567678492c4b97c24644a15f3f29fc4d723b183522
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants