Page MenuHomePhabricator

Document method for reviewers to verify chainTxData
ClosedPublic

Authored by nakihito on Mar 19 2019, 18:46.

Details

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: https://github.com/bitcoin/bitcoin/pull/12317/

Completes T566.

Test Plan

make check
test_runner.py rpc_blockchain

Diff Detail

Repository
rABC Bitcoin ABC
Branch
BackportPR12317
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5251
Build 8565: Bitcoin ABC Buildbot (legacy)
Build 8564: arc lint + arc unit

Event Timeline

nakihito created this revision.Mar 19 2019, 18:46
Owners added a reviewer: Restricted Owners Package.Mar 19 2019, 18:46
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 19 2019, 18:46
Herald added a subscriber: schancel. · View Herald Transcript
nakihito edited the summary of this revision. (Show Details)Mar 19 2019, 18:51
nakihito edited the summary of this revision. (Show Details)Mar 19 2019, 20:06
Fabien requested changes to this revision.Mar 20 2019, 09:53

The new return value from the getchaintxstats RPC may deserve an entry in the release notes.

This also raises a few questions:

  • Do we want to update these params every release or only on major ones like bitcoin core do ?
  • The current tx rate param does not reflect the actual tx rate, but according to getchaintxstats it doesn't seem to match the other actual params either (./src/bitcoin-cli getchaintxstats 4320 000000000000000001d2ce557406b017a928be25ee98906397d339c3f68eec5d returns tx rate of 0.23). Is this intentional ? (And is it the correct window ?)
doc/release-process.md
30

I think you must backport PR12309 first, this will add some context to this sentence: https://github.com/bitcoin/bitcoin/pull/12309

src/rpc/blockchain.cpp
1736

Only porting this line is breaking the help output indent (you can see this through ./src/bitcoin-cli help getchaintxstats).
This is the reason why several lines are modified in the original PR.

1809

Use pushKV instead:
ret.pushKV("window_final_block_hash", pindex->GetBlockHash().GetHex());

This revision now requires changes to proceed.Mar 20 2019, 09:53
jasonbcox added inline comments.Mar 20 2019, 16:57
doc/release-process.md
30

This also needs to be indented since it's in the chainparams section.

nakihito marked 2 inline comments as done.Mar 22 2019, 17:41
nakihito added inline comments.
doc/release-process.md
30

@Fabien I actually reworded the line as it was in this PR to better reflect our update process. As far as I'm aware, we don't update transaction rate or count in chainparams.cpp which makes PR12309's first line irrelevant. I didn't include the second line change in 12309 for similar reasons.

@jasonbcox This line should be indented to be in line with line 29? Should the next line (31) be indented past that or the same?

jasonbcox added inline comments.Mar 25 2019, 17:28
doc/release-process.md
30

Nvm. The above block only refers to updating chainparams' assume valid and chain work. It's fine as-is.

nakihito marked an inline comment as not done.Mar 25 2019, 18:23
nakihito updated this revision to Diff 7816.Mar 25 2019, 18:26

Fixed help text indentation and replaced .push_back() with .pushKV().

jasonbcox requested changes to this revision.Mar 25 2019, 19:48

I'm not sure if this should be part of our release process.

doc/release-process.md
30

The more I look into this, the less valuable it looks to me. The only use of chainTxData is for guessing verification progress in the wallet. @deadalnix do you see value in bloating our release process to add updating chainTxData? I don't.

This revision now requires changes to proceed.Mar 25 2019, 19:48
nakihito updated this revision to Diff 7919.Apr 2 2019, 20:24

Rebased and removed irrelevant addition to release-process.md.

jasonbcox accepted this revision.Apr 2 2019, 20:56
Fabien accepted this revision.Apr 3 2019, 14:44

Updating the txrate can be added later to the release process with no drawback, and I agree with Jason that it has low value.

This revision is now accepted and ready to land.Apr 3 2019, 14:44
This revision was automatically updated to reflect the committed changes.