ISTM the getchaintxstats fields should be window_final_block_hash rather than window_last_block_hash. While here, replace getblockchaininfo with getblockheader (and getblockhash) for updating the nMinimumChainWork and defaultAssumeValid consensus params, update the example PR, and improve a link with a named anchor tag.
Markdown rendering here: https://github.com/jonatack/bitcoin/blob/release-process-getchaintxstats-fix/doc/release-process.md
ACK
ACK 47aaa7df599479b363bceeb6b052726150d9003a 📎
27 | @@ -28,10 +28,10 @@ Release Process
28 |
29 | * Update hardcoded [seeds](/contrib/seeds/README.md), see [this pull request](https://github.com/bitcoin/bitcoin/pull/7415) for an example.
30 | * Update [`src/chainparams.cpp`](/src/chainparams.cpp) m_assumed_blockchain_size and m_assumed_chain_state_size with the current size plus some overhead (see [this](#how-to-calculate-m_assumed_blockchain_size-and-m_assumed_chain_state_size) for information on how to calculate them).
Note: this link seems non-existant
Oh, true, interesting that clicking the link still jumps down to the intended section 👀 obviously this wouldn't work as soon as the section headline text changes -- I guess using a named anchor would be the better way, apparently that's possible with HTML in markdown: https://stackoverflow.com/questions/5319754/cross-reference-named-anchor-in-markdown/7335259#7335259 (untested)
Done (thanks!)
33 | -* Update `src/chainparams.cpp` nMinimumChainWork with information from the getblockchaininfo rpc. 34 | -* Update `src/chainparams.cpp` defaultAssumeValid with information from the getblockhash rpc. 35 | +* Update `src/chainparams.cpp` chainTxData with statistics about the transaction count and rate. Use the output of the `getchaintxstats` RPC, see 36 | + [this pull request](https://github.com/bitcoin/bitcoin/pull/17002) for an example. Reviewers can verify the results by running `getchaintxstats <window_block_count> <window_final_block_hash>` with the `window_block_count` and `window_final_block_hash` from your output. 37 | +* Update `src/chainparams.cpp` nMinimumChainWork with information from the `getblockchaininfo` RPC. 38 | +* Update `src/chainparams.cpp` defaultAssumeValid with information from the `getblockhash` RPC.
Should both lines 33 and 34 refer to getblockheader instead? e.g. combine the two into:
src/chainparams.cpp nMinimumChainWork, defaultAssumeValid, and the block height comment with information from the getblockheader RPC.
Good idea! Particularly I think getblockchaininfo should not be used here, as it only shows the chainwork for the last block, i.e. it is not useful for potential reviewers of the updated parameters. Maybe refer to getblockheader RPC but also add a note to getblockhash in parantheses, as this is useful to convert block height to block hash? (As pointed out in #20263#pullrequestreview-521252569, starting with the block height for both updating and reviewing the chain parameter updates would feel most natural to me).
Oops, I meant to write getblockheader instead of getblockhash, but you're right to mention both and I like how you did your review of 20263. Done (thanks!)
also:
- use the getblockheader (and getblockhash) RPCs instead of getblockchaininfo
for updating the nMinimumChainWork and defaultAssumeValid consensus params
- use "RPC" consistently
- update the example PR from 17002 to 20263
- improve a link with a named anchor tag
re-ACK e5f3e95a8e277acc54bc377a6b116d60d8c4eb5c