Expose txinwitness for coinbase in JSON form from RPC by rvagg · Pull Request #18826 · bitcoin/bitcoin

rvagg · 2020-04-30T09:53:18Z

Rationale

The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself except for the witness commitment nonce which is stored in the scriptWitness of the coinbase and is not printed. You could manually parse the raw "hex" fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data.

Without the nonce you can't:

calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with
reconstruct the raw block form because you don't have scriptWitness stack associated with the coinbase (although you know how big it will be and can guess the common case of [0x000...000])

I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful.

What

This PR simply makes the txinwitness field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest.

Examples

Common case of a [0x000...000] nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7

      "vin": [
        {
          "coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff",
          "txinwitness": [
            "0000000000000000000000000000000000000000000000000000000000000000"
          ],
          "sequence": 4294967295
        }
      ],
...

Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731

      "vin": [
        {
          "coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000",
          "txinwitness": [
            "5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d"
          ],
          "sequence": 4294967295
        }
      ],
...

Alternatives

This field could be renamed for the coinbase, "witnessnonce" perhaps. It could also be omitted when null/zero (0x000...000).

Tests

This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers.

DrahtBot · 2020-04-30T17:57:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: calculate fees in getblock using BlockUndo data #18772 (rpc: calculate fees in getblock using BlockUndo data by robot-visions)

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.

brakmic · 2020-05-01T09:05:46Z

ACK 2a52c3c4caaf8dbe5c3d406c2fc7b1fd4347d0fd

Built, run and tested on macOS Catalina 10.15.4

Regarding functional tests, I'd recommend to expand wallet_basic.py.
You could, for example, add to the end of run_test method the following lines:

witnesses = tx["decoded"]["vin"][0]['txinwitness']
for witness in witnesses:
    assert_is_hex_string(witness)

promag

Please update some test with the new field.

rvagg · 2020-05-04T02:47:51Z

OK, I figured out how to get it wired up into the functional tests, see latest commit exercising this new field successfully. Thanks @brakmic & @promag, PTAL.

brakmic · 2020-05-04T07:45:49Z

Re-ACK 3a20ee8a1c0f86fb7628240f144d02fb6a32022d

luke-jr · 2020-05-04T15:44:43Z

utACK

test/functional/wallet_basic.py

maflcko

ACK, just a nit on the test

test/functional/wallet_basic.py

rvagg · 2020-05-05T04:29:16Z

Moved to feature_segwit.py as suggested. It's a more logical place for it to be, but there's not really anything else in there testing anything similar so it doesn't seem perfectly in place. But perhaps that's just a matter of increasing the coverage in general.

txinwitness is used as the witness commitment nonce so is necessary if reconstructing block data from RPC data.

rvagg · 2020-05-08T02:21:25Z

Squashed the fixups and rebased to master, I think there was a rebasing problem previously and I ended up with stray commits. It should just be the two commits in here now and should be ready for landing if there are no further suggestions or objections.

maflcko · 2020-05-08T11:02:23Z

ACK 34645c4

rvagg · 2020-05-27T05:29:08Z

Wondering what the process is for getting things merged in this project? This is not super urgent but it'd be nice to know if it's in a process and not going to get lost.

maflcko · 2020-05-27T11:41:53Z

@rvagg There is a very loose process described in CONTRIBUTING.md

I was hoping that someone else reviews this, because it is a relatively easy rpc change. If no one else reviews it, I will merge it at some point because it is a clear improvement.

…m RPC 34645c4 Test txinwitness is accessible on coinbase vin (Rod Vagg) 3e44210 Expose txinwitness for coinbase in JSON form (Rod Vagg) Pull request description: ## Rationale The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself **except** for the witness commitment nonce which is stored in the `scriptWitness` of the coinbase and is not printed. You could manually parse the raw `"hex"` fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data. Without the nonce you can't: 1. calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with 2. reconstruct the raw block form because you don't have `scriptWitness` stack associated with the coinbase (although you know how big it will be and can guess the common case of `[0x000...000]`) I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful. ## What This PR simply makes the `txinwitness` field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest. ## Examples Common case of a `[0x000...000]` nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7 ```json "vin": [ { "coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff", "txinwitness": [ "0000000000000000000000000000000000000000000000000000000000000000" ], "sequence": 4294967295 } ], ... ``` Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731 ```json "vin": [ { "coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000", "txinwitness": [ "5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d" ], "sequence": 4294967295 } ], ... ``` ## Alternatives This field could be renamed for the coinbase, `"witnessnonce"` perhaps. It could also be omitted when null/zero (`0x000...000`). ## Tests This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers. ACKs for top commit: MarcoFalke: ACK 34645c4 Tree-SHA512: b192facc1dfd210a5ec3f0d5d1ac6d0cae81eb35be15eaa71f60009a538dd6a79ab396f218434e7e998563f7f0df2c396cc925cb91619f6841c5a67806148c85

laanwj added the RPC/REST/ZMQ label Apr 30, 2020

DrahtBot mentioned this pull request Apr 30, 2020

rpc: calculate fees in getblock using BlockUndo data #18772

Merged

promag reviewed May 3, 2020

View reviewed changes

maflcko reviewed May 4, 2020

View reviewed changes

test/functional/wallet_basic.py Outdated Show resolved Hide resolved

maflcko approved these changes May 4, 2020

View reviewed changes

test/functional/wallet_basic.py Outdated Show resolved Hide resolved

rvagg force-pushed the rvagg/txinwitness-for-coinbase branch from 38feda6 to 959a17d Compare May 5, 2020 06:02

DrahtBot mentioned this pull request May 7, 2020

wallet: always do avoid partial spends if fees are within a specified range #14582

Merged

rvagg added 2 commits May 8, 2020 12:19

Expose txinwitness for coinbase in JSON form

3e44210

txinwitness is used as the witness commitment nonce so is necessary if reconstructing block data from RPC data.

Test txinwitness is accessible on coinbase vin

34645c4

rvagg force-pushed the rvagg/txinwitness-for-coinbase branch from 959a17d to 34645c4 Compare May 8, 2020 02:20

maflcko merged commit 297466b into bitcoin:master Jun 8, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

rvagg commented Apr 30, 2020

Rationale

What

Examples

Alternatives

Tests

Uh oh!

DrahtBot commented Apr 30, 2020

Conflicts

Uh oh!

brakmic commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

rvagg commented May 4, 2020

Uh oh!

brakmic commented May 4, 2020

Uh oh!

luke-jr commented May 4, 2020

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rvagg commented May 5, 2020

Uh oh!

rvagg commented May 8, 2020

Uh oh!

maflcko commented May 8, 2020

Uh oh!

rvagg commented May 27, 2020

Uh oh!

maflcko commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

brakmic commented May 1, 2020 •

edited

Loading