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 Passed
Unit
No Test Coverage
Build Status
Buildable 5366
Build 8794: Bitcoin ABC Buildbot (legacy)
Build 8793: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 19 2019, 18:46
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 ↗(On Diff #7751)

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 ↗(On Diff #7751)

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 ↗(On Diff #7751)

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

This revision now requires changes to proceed.Mar 20 2019, 09:53
doc/release-process.md
30 ↗(On Diff #7751)

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

nakihito added inline comments.
doc/release-process.md
30 ↗(On Diff #7751)

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

doc/release-process.md
30 ↗(On Diff #7751)

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

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 ↗(On Diff #7751)

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

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

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.