[Bundle 4.5/n] Followup fixups to bundle 4 #21525

pull dongcarl wants to merge 10 commits into bitcoin:master from dongcarl:2021-03-kernel-bundle-4.5 changing 15 files +148 −115
  1. dongcarl commented at 7:39 pm on March 24, 2021: member

    Chronological history of this changeset:

    1. Bundle 4 (#21270) got merged
    2. Posthumous reviews were posted
    3. These changes were prepended in bundle 5
    4. More reviews were added in bundle 5
    5. Someone suggested that we split the prepended changes up to another PR
    6. This is that PR

    In the future, I will just do posthumous review changes in another PR instead. I apologize for the confusion.

    Addresses posthumous reviews on bundle 4:

    Addresses reviews on bundle 5:

  2. Revert "miner: Remove old CreateNewBlock w/o chainstate param" 
    This reverts commit 2afcf24408b4453e4418ebfb326b141f6ea8647c.
    0c1b2bc549
  3. Revert "scripted-diff: Invoke CreateNewBlock with chainstate" 
    This reverts commit 46b7f29340acb399fbd2378508a204d8d8ee8fca.
    eede0647b0
  4. Revert "miner: Pass in chainstate to BlockAssembler::CreateNewBlock" 
    This reverts commit d0de61b764fc7e9c670b69d8210705da296dd245.
    e62067e7bc
  5. miner: Add chainstate member to BlockAssembler 7b8e976cd5
  6. node/coinstats: Replace #include with fwd-declaration 07156eb387
  7. dongcarl force-pushed on Mar 24, 2021
  8. DrahtBot added the label Mining on Mar 24, 2021
  9. DrahtBot added the label P2P on Mar 24, 2021
  10. DrahtBot added the label RPC/REST/ZMQ on Mar 24, 2021
  11. DrahtBot added the label Validation on Mar 24, 2021
  12. dongcarl commented at 8:36 pm on March 24, 2021: member

    @ryanofsky w/re #21391 (review) I’ve simplified the changes a bit and it may be a bit easier to review now, see commits:

    • 3460a07d88d841c009e7b7efc0be2ae89bb4a63e
    • 3b75ec491a68eb6fb137c2f829e7dde93b3c3189
  13. DrahtBot commented at 9:38 pm on March 24, 2021: 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:

    • #21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
    • #20295 (rpc: getblockfrompeer by Sjors)

    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.

  14. net_processing: Move comments to declarations 
    Also:
- Remove extraneous blank line
    1dd8ed7a84
  15. node: Avoid potential UB by asserting assumptions 88aead263c
  16. validation: Make BlockManager::LookupBlockIndex const 7e8b5ee814
  17. node/ifaces: NodeImpl: Use an accessor for ChainMan 98c4e252f0
  18. node/ifaces: ChainImpl: Use an accessor for ChainMan 693414d271
  19. dongcarl force-pushed on Mar 30, 2021
  20. dongcarl commented at 5:55 pm on March 30, 2021: member

    Pushed 3b75ec491a68eb6fb137c2f829e7dde93b3c3189 -> 693414d27181cf967f787a2ca72344e52c58c7f0

    • Addressed #21391#pullrequestreview-624405217
  21. ryanofsky approved
  22. ryanofsky commented at 6:26 pm on March 30, 2021: member
    Code review ACK 693414d27181cf967f787a2ca72344e52c58c7f0. I reviewed this previously as part of #21391. I am a fan of the increasingly complicated bundle numbering, and kind of hope there in the next round there is some way we can get bundles 5.333333 and 5.666667!
  23. jamesob approved
  24. jamesob commented at 10:41 pm on March 31, 2021: member

    ACK 693414d27181cf967f787a2ca72344e52c58c7f0 (jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f)

    Built locally and read through the code. Everything here is pretty non-controversial/rote. The first four commits just revert the chainstate parameterization of BlockAssembler methods in favor of making the chainstate a member and (presumably) saving on args.

    0Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: Debian clang version 11.1.0-++20210204120158+1fdec59bffc1-1~exp1~20210203230823.159

    0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 693414d27181cf967f787a2ca72344e52c58c7f0 ([`jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f`](https://github.com/jamesob/bitcoin/tree/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f))
4
5Built locally and read through the code. Everything here is pretty non-controversial/rote. The first four commits just revert the chainstate parameterization of BlockAssembler methods in favor of making the chainstate a member and (presumably) saving on args.
6
7<details><summary>Show platform data</summary>
8<p>

    Tested on Linux-4.19.0-16-amd64-x86_64-with-glibc2.28

    Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis –enable-wallet –enable-debug –with-daemon –enable-natpmp-default CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang –disable-shared –with-pic –enable-benchmark=no –with-bignum=no –enable-module-recovery –enable-module-schnorrsig –enable-experimental –no-create –no-recursion

    Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2 i

    Compiler version: Debian clang version 11.1.0-++20210204120158+1fdec59bffc1-1~exp1~20210203230823.159

     0
 1</p></details>
 2
 3-----BEGIN PGP SIGNATURE-----
 4
 5iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmBk+mQACgkQepNdrbLE
 6TwXnUA/9GSL5MVKKVcWTuPlyLnP6c2MQDjXs7M3D4Bs4vBPAyYdHJlpyF9n4AK7A
 7ahAm+91r+enBanyHgMAiovGdMfHcsUgZ5rLTAsaeGuT6rOVNw3EI8J+X19qf7AHW
 8EJqwf7FSQxRi/AFRJ8QiTFDwkrl9TUOLnFdOqiCrMJuOdauplwMihJ5Ry0DkqBaE
 9TxVQTdE5Pt+KxYJ7bLycfmRmm1SJcBGi/S0CUibx475eIHv67D5u2KfU26L+n5mW
1035tH1tIDvZEA2dvl4xxqtbeNdP2f3pG67u2qQiNSZ2leOlK7bi10F2v4cIeIas77
11edUjJR9BMkQZFbyuKphXStNt5hYrlx2tOVe+F+YhaB1P+yArOZGaJGOfthK6KScA
129rpWYazHqobC+Bnh4H2VandHOsyPTf49aMcIJ64psP5i8gl+RCtTbmt+/N8kS+8Z
13R3eApBtQTMuqeXaoHq5GW4FC1PXKIhiczdnvixBjzkXHaale5eMhiwNVku5D7hbu
14QiC9Ck87znK1Tj776WtcMhx/rszMsSRxipdt+CvfnrqemkLqs18SbecxhFknUCQw
1576LX/kjwQLmE5rJj3RGe4+46STWcPQ4VvO5C5xrx8n/FZtu/e9o4tvKal324Dj19
16tHAtJMOG/VgyAdKWv4iVibR9oa5fNitp3OI0cKa/o0BgMXbyKIQ=
17=IU4Y
18-----END PGP SIGNATURE-----
  25. MarcoFalke removed the label Mining on Apr 1, 2021
  26. MarcoFalke removed the label P2P on Apr 1, 2021
  27. MarcoFalke removed the label RPC/REST/ZMQ on Apr 1, 2021
  28. MarcoFalke added the label Refactoring on Apr 1, 2021
  29. in src/test/util/setup_common.cpp:247 in 7b8e976cd5 outdated 
    243@@ -244,7 +244,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
244 {
245     const CChainParams& chainparams = Params();
246     CTxMemPool empty_pool;
247-    CBlock block = BlockAssembler(empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
248+    CBlock block = BlockAssembler(::ChainstateActive(), empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
    MarcoFalke commented at 8:44 am on April 1, 2021:
    Any reason to use the global here and not m_node.chainman->ActiveChainstate()?
  30. in src/test/util/mining.cpp:44 in 7b8e976cd5 outdated 
    40@@ -41,7 +41,7 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
41 std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
42 {
43     auto block = std::make_shared<CBlock>(
44-        BlockAssembler{*Assert(node.mempool), Params()}
45+        BlockAssembler{::ChainstateActive(), *Assert(node.mempool), Params()}
    MarcoFalke commented at 8:44 am on April 1, 2021:
    node.chainman->ActiveChainstate()
  31. in src/test/miner_tests.cpp:47 in 7b8e976cd5 outdated 
    43@@ -44,7 +44,7 @@ BlockAssembler MinerTestingSetup::AssemblerForTest(const CChainParams& params)
44 
45     options.nBlockMaxWeight = MAX_BLOCK_WEIGHT;
46     options.blockMinFeeRate = blockMinFeeRate;
47-    return BlockAssembler(*m_node.mempool, params, options);
48+    return BlockAssembler(::ChainstateActive(), *m_node.mempool, params, options);
    MarcoFalke commented at 8:44 am on April 1, 2021:
    m_node.chainman->ActiveChainstate()
  32. in src/test/blockfilter_index_tests.cpp:65 in 7b8e976cd5 outdated 
    61@@ -62,7 +62,7 @@ CBlock BuildChainTestingSetup::CreateBlock(const CBlockIndex* prev,
62     const CScript& scriptPubKey)
63 {
64     const CChainParams& chainparams = Params();
65-    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(*m_node.mempool, chainparams).CreateNewBlock(scriptPubKey);
66+    std::unique_ptr<CBlockTemplate> pblocktemplate = BlockAssembler(::ChainstateActive(), *m_node.mempool, chainparams).CreateNewBlock(scriptPubKey);
    MarcoFalke commented at 8:44 am on April 1, 2021:
    m_node.chainman->ActiveChainstate()
  33. in src/rpc/mining.cpp:751 in 7b8e976cd5 outdated 
    747@@ -748,7 +748,7 @@ static RPCHelpMan getblocktemplate()
748 
749         // Create new block
750         CScript scriptDummy = CScript() << OP_TRUE;
751-        pblocktemplate = BlockAssembler(mempool, Params()).CreateNewBlock(scriptDummy);
752+        pblocktemplate = BlockAssembler(::ChainstateActive(), mempool, Params()).CreateNewBlock(scriptDummy);
    MarcoFalke commented at 8:45 am on April 1, 2021:
    Any reason to take mempool from the node and the chainstate from the global?
  34. in src/rpc/mining.cpp:361 in 7b8e976cd5 outdated 
    357@@ -358,7 +358,7 @@ static RPCHelpMan generateblock()
358         LOCK(cs_main);
359 
360         CTxMemPool empty_mempool;
361-        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(empty_mempool, chainparams).CreateNewBlock(coinbase_script));
362+        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(::ChainstateActive(), empty_mempool, chainparams).CreateNewBlock(coinbase_script));
    MarcoFalke commented at 8:47 am on April 1, 2021:
    same
  35. in src/net_processing.cpp:506 in 1dd8ed7a84 outdated 
    501+     * @param[in]   start_height    The start height for the request
502+     * @param[in]   stop_hash       The stop_hash for the request
503+     * @param[in]   max_height_diff The maximum number of items permitted to request, as specified in BIP 157
504+     * @param[out]  stop_index      The CBlockIndex for the stop_hash block, if the request can be serviced.
505+     * @param[out]  filter_index    The filter index, if the request can be serviced.
506+     * @return                      True if the request can be serviced.
    MarcoFalke commented at 8:49 am on April 1, 2021:
    Commit body could mention that this can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space?
  36. MarcoFalke commented at 8:58 am on April 1, 2021: member

    Left some suggestions for bundle 4.75

    review ACK 693414d27181cf967f787a2ca72344e52c58c7f0 🛐

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 693414d27181cf967f787a2ca72344e52c58c7f0 🛐
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgGPQwAuhB3nTeI9D7jHjXnmRdMdr3YUwXpCQTpDGxysZm7t2S4vmSsJajBJDN5
 8e6CYQj/Bi0ZwTognO4SBoxkf9jJBJl/pKtNOrdDr9CEwVUkLTFOSkYCLclg40Vw5
 9z7FmMX6GsO6YIs5rZhJrQcFfeGAtRiHjW4FBZ397xCr9ol32HZJZoN7TIBShmYda
10Q1KHZb2bwDZdkjBFVI8jatIsIwioVRJZ99EU2FxFX6K6Ma36uDXVFLiRDcHK3mKe
11HFzChWEP0DOu7Soy9NY2aproiPwiV7y+YZXcgNF9DWRrzGVFifO5fqx+xIimxalh
12lRsPUqULdZf2GF0+TGar1w/NC7HAU5tBdbQDO04twYBe3N6+1Wg8yNYfMnb6JmGH
13LWAU7JTAQ3t+x7iwYN5N+46fTFrair3j5y/NNCnsOBnH8TDvO/xEDlBgE/30fArG
14gz/n/BZ6FAwJb7bZDXdAJBVVmYEDSmCL/fh6Efwg+64ODtHBfFE/dKiQS+uA4JW3
15KOBu1ZhV
16=fRev
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 011488e624ba1f6bc94c50a176ab89a375fd25e7786cc3f970aac8152f9c753b -

  37. MarcoFalke approved
  38. MarcoFalke merged this on Apr 1, 2021
  39. MarcoFalke closed this on Apr 1, 2021
  40. in src/miner.h:205 in 7b8e976cd5 outdated 
    201@@ -201,6 +202,7 @@ class BlockAssembler
202 void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
203 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
204 
205+// TODO just accept a CBlockIndex*
    MarcoFalke commented at 9:04 am on April 1, 2021:
    I don’t understand this TODO. CBlockIndex requires the block to be in the blockchain (block tree). However the coinbase commitment is regenerated before the block is submitted to the tree.
    MarcoFalke commented at 6:14 am on April 3, 2021:
    (Reply #21391 (comment))
  41. sidhujag referenced this in commit 72fff53664 on Apr 1, 2021
  42. MarcoFalke referenced this in commit 0dd7b23489 on Apr 17, 2021
  43. Fabcien referenced this in commit 9813976d76 on May 9, 2022
  44. Fabcien referenced this in commit a701d5196e on May 9, 2022
  45. Fabcien referenced this in commit 01a7dd7dc3 on May 9, 2022
  46. Fabcien referenced this in commit bea681fd0d on May 9, 2022
  47. Fabcien referenced this in commit 1b2cb7e3cd on May 23, 2022
  48. Fabcien referenced this in commit a75ac0bf64 on May 23, 2022
  49. DrahtBot locked this on Aug 16, 2022


dongcarl DrahtBot ryanofsky jamesob MarcoFalke

Labels
Refactoring Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me