Expose txinwitness for coinbase in JSON form from RPC #18826

rvagg commented at 9:53 AM on April 30, 2020: contributor

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.

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

DrahtBot commented at 5:57 PM on April 30, 2020: 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:

#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 commented at 9:05 AM on May 1, 2020: contributor

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 commented at 11:51 PM on May 3, 2020: member

Please update some test with the new field.

rvagg commented at 2:47 AM on May 4, 2020: contributor

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 commented at 7:45 AM on May 4, 2020: contributor

Re-ACK 3a20ee8a1c0f86fb7628240f144d02fb6a32022d

luke-jr commented at 3:44 PM on May 4, 2020: member

utACK

in test/functional/wallet_basic.py:435 in 3a20ee8a1c outdated

 428 | @@ -428,6 +429,15 @@ def run_test(self):
 429 |          assert_equal(coinbase_tx_1["transactions"][0]["blockhash"], blocks[1])
 430 |          assert_equal(len(self.nodes[0].listsinceblock(blocks[1])["transactions"]), 0)
 431 |  
 432 | +        # Check that we can retrieve the witness nonce from the coinbase
 433 | +        coinbase_txid = self.nodes[0].getblock(block_hash)['tx'][0]
 434 | +        coinbase_tx_2 = self.nodes[1].gettransaction(txid=coinbase_txid, verbose=True)
 435 | +        assert "txinwitness" in coinbase_tx_2["decoded"]["vin"][0]

MarcoFalke commented at 11:17 PM on May 4, 2020:

not sure why this in in the wallet test. It seems blockchain or segwit related. You can put it in ./test/functional/feature_segwit.py (or ./test/functional/rpc_blockchain.py if you prefer).

Also, this assert can be left out. The next line will throw KeyError and fail the test when txinwitness is not found.

in test/functional/wallet_basic.py:439 in 3a20ee8a1c outdated

 434 | +        coinbase_tx_2 = self.nodes[1].gettransaction(txid=coinbase_txid, verbose=True)
 435 | +        assert "txinwitness" in coinbase_tx_2["decoded"]["vin"][0]
 436 | +        witnesses = coinbase_tx_2["decoded"]["vin"][0]["txinwitness"]
 437 | +        assert_equal(len(witnesses), 1)
 438 | +        assert_is_hex_string(witnesses[0])
 439 | +        assert_equal(len(witnesses[0]), 64)

MarcoFalke commented at 11:19 PM on May 4, 2020:

        assert_equal(witnesses[0], '00'*32)

I think Bitcoin Core uses all zeros as the nonce by default, so you could just directly check this here. Though, your check also works.

rvagg commented at 4:07 AM on May 5, 2020:

Yep, it does, and I think I'll switch to your explicit check -- if Bitcoin Core ever changes to some other strategy then having to update the tests as an explicit acknowledgement of that would probably be a good idea. Thanks.

MarcoFalke approved

MarcoFalke commented at 11:19 PM on May 4, 2020: member

ACK, just a nit on the test

rvagg commented at 4:29 AM on May 5, 2020: contributor

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.

rvagg force-pushed on May 5, 2020

Expose txinwitness for coinbase in JSON form

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

3e4421070a

Test txinwitness is accessible on coinbase vin 34645c4dd0

rvagg force-pushed on May 8, 2020

rvagg commented at 2:21 AM on May 8, 2020: contributor

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.

MarcoFalke commented at 11:02 AM on May 8, 2020: member

ACK 34645c4dd04f1e9bc199fb722de0bb397ec0e131

rvagg commented at 5:29 AM on May 27, 2020: contributor

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.

MarcoFalke commented at 11:41 AM on May 27, 2020: member

@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.

MarcoFalke merged this on Jun 8, 2020

MarcoFalke closed this on Jun 8, 2020

sidhujag referenced this in commit 009eafa46f on Jun 8, 2020

DrahtBot locked this on Feb 15, 2022