Document method for reviewers to verify chainTxData #12317

jnewbery · 2018-01-31T18:50:08Z

This commit adds the final block hash of the window to getchaintxstats
and documents how reviewers can verify changes to chainTxData.

maflcko · 2018-01-31T18:55:58Z

The block hash is shown in the diff on the pull request. I think that implies that you as a reviewer go ahead and type in "getchaintxstats (30 * 24 * 6) $that_hash". No need to be overly verbose, imo.

In the worst case they would run getchaintxstats without any args and get a similar result.

jnewbery · 2018-01-31T19:04:57Z

In the worst case they would run getchaintxstats without any args and get a similar result.

Here's @laanwj 's getchaintxstats from #12270:

{
  "time": 1516903077,
  "txcount": 295363220,
  "window_block_count": 4320,
  "window_tx_count": 7882762,
  "window_interval": 2242748,
  "txrate": 3.514778298765621
}

and mine when I check it a few days later:

→ bcli getchaintxstats
{
  "time": 1517425124,
  "txcount": 296682224,
  "window_block_count": 4320,
  "window_tx_count": 7270004,
  "window_interval": 2273816,
  "txrate": 3.197270139712272
}

so txrate is ~10% different. I didn't ACK that PR because I didn't think that there was a way to verify it.

maflcko · 2018-01-31T22:01:50Z

Maybe we should take the max for txrate, since it fluctuates so much. Or take a larger interval for that?

jnewbery · 2018-01-31T22:13:51Z

Maybe we should take the max for txrate, since it fluctuates so much. Or take a larger interval for that?

Perhaps, but I think that's a separate issue from this.

I'm just looking for a way that reviewers can quickly and easily verify the update to chainTxData.

laanwj · 2018-02-01T08:11:42Z

utACK, TIL that getchaintxstats takes arguments at all.

promag

Could have 2 tests:

window_final_block_hash is in the response and it's a block hash
window_final_block_hash matches blockhash parameter when specified.

promag · 2018-02-02T12:20:00Z

src/rpc/blockchain.cpp

Sentence case. Missing period.

This commit adds the final block hash of the window to getchaintxstats and documents how reviewers can verify changes to chainTxData.

jnewbery · 2018-02-02T14:30:14Z

Addressed @promag nits

promag · 2018-02-02T15:08:33Z

utACK 7444149.

jamesob · 2018-02-02T16:37:50Z

utACK 7444149

7444149 Document method for reviewers to verify chainTxData (John Newbery) Pull request description: This commit adds the final block hash of the window to getchaintxstats and documents how reviewers can verify changes to chainTxData. Tree-SHA512: d16abb5f47d058e52660f4d495f1e453205b1b83716d7c810ff62a70338db721386c1808ec1fc8468f514e4d80cc58e3c96eeb3184cbbcb1d07830fa5e53f342

meshcollider · 2018-02-02T19:48:35Z

Post-merge-utACK 7444149

Summary: This commit adds the final block hash of the window to getchaintxstats and documents how reviewers can verify changes to chainTxData. Backport of PR12317: bitcoin/bitcoin#12317 Completes T566. Test Plan: make check test_runner.py rpc_blockchain Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, Fabien, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2711

@luke-jr

* Merge #9622: [rpc] listsinceblock should include lost transactions when parameter is a reorg'd block Summary: 876e92b Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. (Karl-Johan Alm) f999c46 listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. (Karl-Johan Alm) Pull request description: The following scenario will not notify the caller of the fact `tx0` has been dropped: 1. User 1 receives BTC in tx0 from utxo1 in block aa1. 2. User 2 receives BTC in tx1 from utxo1 (same) in block bb1 3. User 1 sees 2 confirmations at block aa3. 4. Reorg into bb chain. 5. User 1 asks `listsinceblock aa3` and does not see that tx0 is now invalidated. See `listsinceblock.py` commit for related test. The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the `listsinceblock` output in a new `replaced` array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe. Example output: ```Python { 'transactions': [], 'replaced': [ { 'walletconflicts': [], 'vout': 1, 'account': '', 'timereceived': 1485234857, 'time': 1485234857, 'amount': '1.00000000', 'bip125-replaceable': 'unknown', 'trusted': False, 'category': 'receive', 'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff', 'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ', 'label': '', 'confirmations': -7 } ], 'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715' } ``` I believe this addresses the comment by @luke-jr in bitcoin/bitcoin#9516 (comment) but I could be wrong.. Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35 Backport of Core PR 9622 https://github.com/bitcoin/bitcoin/pull/9622/files Completes T551 Test Plan: make check test_runner.py wallet_listsinceblock Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2689 * Fix unlocked_until in getwalletinfo rpc Summary: Getwalletinfo returned an uninitialized value for unlocked_until before the first walletpassphrase call, even though the wallet was locked and it should return 0. Test Plan: make check ./wallet_encryption.py Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2633 * Remove nonnull warning when calling secp256k1_schnorr_sign with NULL noncefp Summary: This warning was hidden under normal conditions because SECP256K1_ARG_NONNULL is bypassed when SECP256K1_BUILD is set. A NULL noncefp means the default nonce function is used. Test Plan: make check Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2747 * [Part 2 of 5] Add a CChainState class to clarify internal interfaces Summary: Move block writing out of AcceptBlock Backport of Core PR 10279 commit e104f0f bitcoin/bitcoin@e104f0f Depends on D1968 Progress towards T572 Test Plan: make check test_runner.py Reviewers: #bitcoin_abc, schancel, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: jasonbcox, teamcity Differential Revision: https://reviews.bitcoinabc.org/D1969 * [secp256k1] add schnorr verify jni binding Summary: schnorr jni binding that will allow jvm support Test Plan: use the test vector data from https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr/test-vectors.csv Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox Subscribers: jasonbcox, schancel Differential Revision: https://reviews.bitcoinabc.org/D2745 * Merge #10858: [RPC] Add "warnings" field to getblockchaininfo and unify "warnings" field in get*info RPCs Summary: 395cef7 Change getmininginfo errors field to warnings (Andrew Chow) 8502b20 Unify help text for GetWarnings output in get*info RPCs (Andrew Chow) f77f0e4 Add warnings field to getblockchaininfo (Andrew Chow) Pull request description: The `getblockchaininfo` output does not contain the `warnings` field which the `getinfo`, `getmininginfo`, and `getnetworkinfo` RPCs have. It should have it as the errors pertain to the blockchain. This PR adds that field. This PR also unifies the help text for the `warnings` field and its output position so that all of the `get*info` commands are consistent. `getnetworkinfo`'s `errors` field is named `warnings`. I did not change this even though it is inconsistent since this naming has been in use for a long time. Tree-SHA512: 385ab6acfee67fc8816f4d51ab2bd7a623264c7973906dfbab0a171f199e9db16fde19093a5bc3dfbdd4ff5f19d2186b646eb6b3bae0a4d7c9add43650a4a9d9 Backport of PR10858 bitcoin/bitcoin@9a8e916?diff=unified#diff-a0c8f511d90e83aa9b5857e819ced344R1165 Completes T567 Test Plan: make check test_runner.py getblockchaininfo should also produce a "warnings" field getmininginfo should have a "warnings" field in place of its "errors" field during regular use getmininginfo should behave as previously (i.e. have an "errors" field) when running bitcoind -deprecatedrpc=getmininginfo Reviewers: deadalnix, jasonbcox, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, Fabien, O1 Bitcoin ABC, #bitcoin_abc Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2730 * remove unused fnoncriticalerrors variable from cwalletdb::findwallettx Summary: remove unused fnoncriticalerrors variable from cwalletdb::findwallettx Backport of Core PR11923 https://github.com/bitcoin/bitcoin/pull/11923/files Completes T570 Test Plan: make check test_runner.py bitcoin-qt Reviewers: Fabien, jasonbcox, deadalnix, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2742 * Fix comment about s in schnorr sigs Summary: s/order/n/ for consistency. Test Plan: None Reviewers: markblundeberg, O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2741 * [Part 3 of 5] Add a CChainState class to clarify internal interfaces Summary: Make DisconnectBlock unaware of where undo data resides on disk Backport of core PR 10279 Commit 93a34cf. Depends on D1969 Test Plan: make VERBOSE=1 check && ./test/functional/test_runner.py Reviewers: #bitcoin_abc, O1 Bitcoin ABC, jasonbcox Reviewed By: #bitcoin_abc, O1 Bitcoin ABC, jasonbcox Subscribers: teamcity Differential Revision: https://reviews.bitcoinabc.org/D1970 * Lint everything Summary: As per title Test Plan: make check ./test/functional/test_runner.py Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2758 * Merge #10275: [rpc] Allow fetching tx directly from specified block in getrawtransaction Summary: 434526a [test] Add tests for getrawtransaction with block hash. (Karl-Johan Alm) b167951 [rpc] Allow getrawtransaction to take optional blockhash to fetch transaction from a block directly. (Karl-Johan Alm) a5f5a2c [rpc] Fix fVerbose parsing (remove excess if cases). (Karl-Johan Alm) Pull request description: [Reviewer hint: use [?w=1](https://github.com/bitcoin/bitcoin/pull/10275/files?w=1) to avoid seeing a bunch of indentation changes.] Presuming a user knows the block hash of the block containing a given transaction, this PR allows them to fetch the raw transaction, even without `-txindex`. It also enables support for getting transactions that are in orphaned blocks. Note that supplying a block hash will override mempool and txindex support in `GetTransaction`. The rationale behind this is that a transaction may be in multiple places (orphaned blocks) and if the user supplies an explicit block hash it should be adhered to. ```Bash $ # a41.. is a tx inside an orphan block ..3c6f.. -- first try getting it normally $ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 error code: -5 error message: No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions. $ # now try with block hash $ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 0000000000000000003c6fe479122bfa4a9187493937af1734e1e5cd9f198ec7 { "hex": "01000000014e7e81144e42f6d65550e59b715d470c9301fd7ac189[...]90488ac00000000", "inMainChain": false, "txid": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79", "hash": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79", "size": 225, [...] } $ # another tx 6c66... in block 462000 $ ./bitcoin-cli getrawtransaction 6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735 1 00000000000000000217f2c12922e321f6d4aa933ce88005a9a493c503054a40 { "hex": "0200000004d157[...]88acaf0c0700", "inMainChain": true, "txid": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735", "hash": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735", "size": 666, [...] } $ ``` Tree-SHA512: 279be3818141edd3cc194a9ee65929331920afb30297ab2d6da07293a2d7311afee5c8b00c6457477d9f1f86e86786a9b56878ea3ee19fa2629b829d042d0cda Backport of Core PR 10275 https://github.com/bitcoin/bitcoin/pull/10275/files Completes T562 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, #bitcoin_abc Reviewed By: Fabien, #bitcoin_abc Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2746 * Backport remaining changes from Core PR 10742 Summary: Partial backport of Core PR 10742 https://github.com/bitcoin/bitcoin/pull/10742/files Completes T549 Test Plan: make check test_runner.py Reviewers: #bitcoin_abc, deadalnix, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2719 * Use static_cast instead of C-style casts for non-fundamental types Summary: A C-style cast is equivalent to try casting in the following order: 1. const_cast(...) 2. static_cast(...) 3. const_cast(static_cast(...)) 4. reinterpret_cast(...) 5. const_cast(reinterpret_cast(...)) By using static_cast<T>(...) explicitly we avoid the possibility of an unintentional and dangerous reinterpret_cast. Furthermore static_cast<T>(...) allows for easier grepping of casts. Please enter the commit message for your changes. Lines starting Backport of Core PR10498 bitcoin/bitcoin#10498 Completes T555 Test Plan: make check test_runner.py Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2765 * Fix Windows build errors introduced in D2765 Summary: See core issue #12386: bitcoin/bitcoin#12386 Backport of core PR12416 Test Plan: make check Build the windows binaries Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2779 * [Target v0.19] Deprecate and add test for signrawtransaction Summary: Deprecate signrawtransaction (target is v0.19). Backport 3/3 of core PR10579 (part of commit d602348 + deprecation from 1e79c05) Depends on D2763 Completes task T438 Test Plan: make check ./test/functional/test_runner.py --extended Reviewers: #bitcoin_abc, jasonbcox, deadalnix Reviewed By: #bitcoin_abc, jasonbcox, deadalnix Subscribers: deadalnix, teamcity Differential Revision: https://reviews.bitcoinabc.org/D2009 * Add schnorr sign benchmark Summary: As per title. Depends on D2747. Test Plan: ./bench_sign Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien, jasonbcox Subscribers: jasonbcox, Fabien, schancel Differential Revision: https://reviews.bitcoinabc.org/D2748 * Add schnorr verify benchmark Summary: As per title. Depends on D2747. Test Plan: ./bench_verify Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien, jasonbcox Subscribers: jasonbcox, Fabien, schancel Differential Revision: https://reviews.bitcoinabc.org/D2749 * Update wallet_listsinceblock.py to use signrawtransactionwithwallet rpc Summary: This has been added before D2009 gets landed, it needs to be updated as `signrawtransaction` is going to get deprecated in favor of `signrawtransactionwithwallet` and `signrawtransactionwithkey`. Test Plan: ./test/functional/test_runner.py wallet_listsinceblock Reviewers: #bitcoin_abc, deadalnix, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: schancel Differential Revision: https://reviews.bitcoinabc.org/D2763 * Document method for reviewers to verify chainTxData Summary: This commit adds the final block hash of the window to getchaintxstats and documents how reviewers can verify changes to chainTxData. Backport of PR12317: bitcoin/bitcoin#12317 Completes T566. Test Plan: make check test_runner.py rpc_blockchain Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, Fabien, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2711 * qa: Improve getchaintxstats functional test Summary: Completes T561. Depends on D2711. Backport of PR12083 https://github.com/bitcoin/bitcoin/pull/12083/files Test Plan: make check test_runner.py Reviewers: Fabien, deadalnix, O1 Bitcoin ABC, #bitcoin_abc, nakihito Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Subscribers: teamcity, schancel Differential Revision: https://reviews.bitcoinabc.org/D2702

7444149 Document method for reviewers to verify chainTxData (John Newbery) Pull request description: This commit adds the final block hash of the window to getchaintxstats and documents how reviewers can verify changes to chainTxData. Tree-SHA512: d16abb5f47d058e52660f4d495f1e453205b1b83716d7c810ff62a70338db721386c1808ec1fc8468f514e4d80cc58e3c96eeb3184cbbcb1d07830fa5e53f342

maflcko added the Docs label Jan 31, 2018

promag reviewed Feb 2, 2018

View reviewed changes

src/rpc/blockchain.cpp Outdated

Copy link

Contributor

promag Feb 2, 2018

Choose a reason for hiding this comment

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

Sentence case. Missing period.

Document method for reviewers to verify chainTxData

7444149

This commit adds the final block hash of the window to getchaintxstats and documents how reviewers can verify changes to chainTxData.

jnewbery force-pushed the verify_chainTxData branch from 0d0bef5 to 7444149 Compare February 2, 2018 14:29

laanwj merged commit 7444149 into bitcoin:master Feb 2, 2018

jnewbery deleted the verify_chainTxData branch February 2, 2018 17:40

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Document method for reviewers to verify chainTxData #12317

Document method for reviewers to verify chainTxData #12317

Uh oh!

jnewbery commented Jan 31, 2018

Uh oh!

maflcko commented Jan 31, 2018

Uh oh!

jnewbery commented Jan 31, 2018

Uh oh!

maflcko commented Jan 31, 2018

Uh oh!

jnewbery commented Jan 31, 2018

Uh oh!

laanwj commented Feb 1, 2018

Uh oh!

promag left a comment

Uh oh!

promag Feb 2, 2018

Uh oh!

jnewbery commented Feb 2, 2018

Uh oh!

promag commented Feb 2, 2018

Uh oh!

jamesob commented Feb 2, 2018

Uh oh!

meshcollider commented Feb 2, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Document method for reviewers to verify chainTxData #12317

Document method for reviewers to verify chainTxData #12317

Uh oh!

Conversation

jnewbery commented Jan 31, 2018

Uh oh!

maflcko commented Jan 31, 2018

Uh oh!

jnewbery commented Jan 31, 2018

Uh oh!

maflcko commented Jan 31, 2018

Uh oh!

jnewbery commented Jan 31, 2018

Uh oh!

laanwj commented Feb 1, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 2, 2018

Uh oh!

promag commented Feb 2, 2018

Uh oh!

jamesob commented Feb 2, 2018

Uh oh!

meshcollider commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

meshcollider commented Feb 2, 2018 •

edited

Loading