Make g_chainman internal to validation #18698

pull MarcoFalke wants to merge 6 commits into bitcoin:master from MarcoFalke:2004-chainman changing 17 files +161 −134
  1. MarcoFalke commented at 1:31 PM on April 18, 2020: member

    The global g_chainman has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.

    The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.

    I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.

  2. fanquake added the label Refactoring on Apr 18, 2020
  3. MarcoFalke force-pushed on Apr 18, 2020
  4. DrahtBot commented at 4:45 PM on April 18, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18988 (RFC: Introducing AltNet, a pluggable framework for alternative transports by ariard)
    • #18987 (RFC: Introducing Watchdog, a cross-layer anomaly detection module by ariard)
    • #18933 (rpc: Add submit option to generateblock by MarcoFalke)
    • #18740 (Remove g_rpc_node global by ryanofsky)
    • #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
    • #18638 (net: Use mockable time for ping/pong, add tests by MarcoFalke)
    • #18637 (coins: allow cache resize after init by jamesob)
    • #18354 (Protect wallet by using shared pointers by bvbfan)
    • #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
    • #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)
    • #15367 (feature: Added ability for users to add a startup command by benthecarman)
    • #15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)
    • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

    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.

  5. practicalswift commented at 4:54 PM on April 18, 2020: contributor

    Concept ACK: very welcome from a fuzzing perspective!

    I know that I am repeating myself but nevertheless: thanks for doing these coupling cleanups :)

  6. DrahtBot cross-referenced this on Apr 18, 2020 from issue rpc: remove g_rpc_node & g_rpc_chain by brakmic
  7. DrahtBot cross-referenced this on Apr 18, 2020 from issue Prevent consecutive ::cs_main locks by hebasto
  8. DrahtBot cross-referenced this on Apr 18, 2020 from issue net: Use mockable time for ping/pong, add tests by MarcoFalke
  9. DrahtBot cross-referenced this on Apr 18, 2020 from issue coins: allow cache resize after init by jamesob
  10. DrahtBot cross-referenced this on Apr 18, 2020 from issue Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery
  11. DrahtBot cross-referenced this on Apr 18, 2020 from issue Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard
  12. DrahtBot cross-referenced this on Apr 18, 2020 from issue feature: Added ability for users to add a startup command by benthecarman
  13. MarcoFalke cross-referenced this on Apr 20, 2020 from issue assumeutxo by jamesob
  14. DrahtBot cross-referenced this on Apr 20, 2020 from issue Add local thread pool to CCheckQueue by hebasto
  15. DrahtBot cross-referenced this on Apr 22, 2020 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
  16. DrahtBot cross-referenced this on Apr 22, 2020 from issue refactor: Make CCheckQueue RAII-styled by hebasto
  17. DrahtBot cross-referenced this on Apr 22, 2020 from issue miner: Avoid stack-use-after-return in validationinterface by MarcoFalke
  18. DrahtBot cross-referenced this on Apr 22, 2020 from issue Remove g_rpc_node global by ryanofsky
  19. DrahtBot cross-referenced this on Apr 23, 2020 from issue Add fee_est tool for debugging fee estimation code by ryanofsky
  20. DrahtBot cross-referenced this on Apr 24, 2020 from issue test: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer by practicalswift
  21. MarcoFalke force-pushed on Apr 24, 2020
  22. MarcoFalke commented at 7:09 PM on April 24, 2020: member

    Rebased due to conflict in tests

  23. DrahtBot cross-referenced this on Apr 28, 2020 from issue wip: Only support shared validation interfaces by promag
  24. DrahtBot cross-referenced this on Apr 28, 2020 from issue Use shared pointers only in validation interface by bvbfan
  25. DrahtBot cross-referenced this on May 1, 2020 from issue test: Set BIP34Height = 2 for regtest by MarcoFalke
  26. in src/init.cpp:1375 in faad46a535 outdated 
    1361 | @@ -1361,6 +1362,9 @@ bool AppInitMain(NodeContext& node)
1362 |      // which are all started after this, may use it from the node context.
1363 |      assert(!node.mempool);
1364 |      node.mempool = &::mempool;
1365 | +    assert(!node.chainman);
1366 | +    node.chainman = &::g_chainman;
    ryanofsky commented at 7:00 AM on May 1, 2020:

    In commit "node: Add chainman alias for g_chainman" (faad46a5358a12691c80c027caf666e91ca85a1e)

    Here and in test setup could consider replacing double redundant ::g_ prefix with just g_. If a variable name already begins with g_ then :: doesn't tell you much and gets in the way of creating namespaces

    MarcoFalke commented at 6:26 PM on May 5, 2020:

    Good point. Will address on the next force push or the follow-up pull, whichever comes earlier.

    MarcoFalke commented at 2:58 PM on May 14, 2020:

    Thanks, fixed.

  27. in src/validation.h:876 in fa24ea2e67 outdated 
     916 | @@ -917,6 +917,7 @@ class ChainstateManager
 917 |      void Reset();
 918 |  };
 919 |  
 920 | +/** DEPRECATED! Please use node.chainman instead. May only be used in validation.cpp internally */
    ryanofsky commented at 7:19 AM on May 1, 2020:

    In commit "validation: Mark g_chainman DEPRECATED" (fa24ea2e67d9812a7b57f9820996a8695a8fef37)

    Could enforce this by deleting extern declaration from validation.h, and just declaring it an extern variable right where it's used in init.cpp.

    MarcoFalke commented at 6:26 PM on May 5, 2020:

    I might remove it in a later pull altogether, so I opted for the smallest diff for now

  28. ryanofsky approved
  29. ryanofsky commented at 7:28 AM on May 1, 2020: contributor

    Code review ACK fa24ea2e67d9812a7b57f9820996a8695a8fef37. Looks all good. Ignore my suggestions below. Would be good to have @jamesob concept ACK

  30. DrahtBot added the label Needs rebase on May 1, 2020
  31. MarcoFalke force-pushed on May 5, 2020
  32. DrahtBot removed the label Needs rebase on May 5, 2020
  33. DrahtBot cross-referenced this on May 6, 2020 from issue Serve cfcheckpt requests by jnewbery
  34. DrahtBot cross-referenced this on May 6, 2020 from issue [WIP] Serve BIP 157 compact filters by jnewbery
  35. jamesob commented at 8:25 PM on May 6, 2020: member

    Concept ACK! Reviewing in detail now.

  36. jamesob approved
  37. jamesob commented at 2:56 PM on May 7, 2020: member

    ACK fa5f7c0b47065476a9ba0a716d9a35c2d0dde2df (jamesob/ackr/18698.1.MarcoFalke.make_g_chainman_internal)

    Built locally. Some commits most easily reviewed with --color-moved=dimmed_zebra --ignore-space-change.

    This is a big step in the right direction. Ultimately it would be great (as @ryanofsky alluded to) to be able to remove the g_chainman symbol completely, but that will require some bigger changes, like parameterizing ChainstateActive() by a node.

    This is an obvious improvement IMO if there are fuzzing benefits.

    <details><summary>Show platform data</summary> <p>

    Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Werror=thread-safety-analysis -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

    </p></details>

    <details><summary>Show signature data</summary> <p>

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK fa5f7c0b47065476a9ba0a716d9a35c2d0dde2df ([`jamesob/ackr/18698.1.MarcoFalke.make_g_chainman_internal`](https://github.com/jamesob/bitcoin/tree/ackr/18698.1.MarcoFalke.make_g_chainman_internal))

Built locally. Some commits most easily reviewed with 
`--color-moved=dimmed_zebra --ignore-space-change`.

This is a big step in the right direction. Ultimately it would be great (as
[@ryanofsky](/bitcoin-bitcoin/contributor/ryanofsky/) alluded to) to be able to remove the `g_chainman` symbol completely,
but that will require some bigger changes, like parameterizing `ChainstateActive()`
by a `node`.

This is an obvious improvement IMO if there are fuzzing benefits.

<details><summary>Show platform data</summary>
<p>

    Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic

    Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion

    Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i

    Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

    
</p></details>
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl60IUYACgkQyrywS1A+
5miwVg/+Plvj8qNPAp5ajpXzpjhxp3aJ3T+BEQL3mssNc7isL9loO6hjpajnuD+X
M8+BreXXBScqE5SurAA88y3hy6YWW00aiktA4EtTXAy5x2R0MOeL71oQ81VCNVV2
ev85rf8TWPD4obiPL8NtZ0AP48s3ttn4rBgFLDXyWR3i097qmDVqqHZcszw+8z/0
R2jT5ocf8XLp+MKIXKrw6h03JAN7LxOssBI1BIX0Da5A1Zp291FxY10W883hUE/d
AR75gwqjTXJPDuoIz4KHtmsEd/6bv/rvk4PI68ktZc0rUxt9oJQxFtaftrmUK5SY
MezBTkX3s6e/zPRM0v2ma8aEQd7l01yvvBoYOFaQdyxtFD0CzoxToE2m9HEgEcaX
dojkVnVdwelT+SG15nwPOMYAs8G9YPW3c6RcS+KD8TlDIk2q5BWoVcGIspU50eZv
bHtnOyJ6Zcyw3AHA5HwfM/0Ft7j0I/uKNuK2qtHkYc0JfNNeMh3I+Hw1R2J1bD0u
jj/idtoZ1RyU9niBWun/7hUIXByeBXJTzR4dM8adwfTtR30T/5PX90wfsndn56DM
RA2LA47NvKQb1h9cZyPyzWic1zQGzBCA/Hf1GCccTFC9hNS1kuax/U7eJYMDLEqR
QCU7JU9b673kvR2e54rsfpI+xYf78izdmnXwJU5xHPVX8k6q3lo=
=up7+
-----END PGP SIGNATURE-----

    </p></details>

  38. DrahtBot cross-referenced this on May 7, 2020 from issue Immediately disconnect on invalid net message checksum by jonasschnelli
  39. DrahtBot cross-referenced this on May 10, 2020 from issue rpc: Add submit option to generateblock by MarcoFalke
  40. DrahtBot added the label Needs rebase on May 12, 2020
  41. ryanofsky approved
  42. ryanofsky commented at 7:10 PM on May 13, 2020: contributor

    Code review ACK fa5f7c0b47065476a9ba0a716d9a35c2d0dde2df. Just rebased since last review (and needs another rebase)

  43. MarcoFalke force-pushed on May 14, 2020
  44. MarcoFalke force-pushed on May 14, 2020
  45. DrahtBot removed the label Needs rebase on May 14, 2020
  46. ryanofsky approved
  47. ryanofsky commented at 10:01 PM on May 14, 2020: contributor

    Code review ACK 9f0a6f36b5ccf08e054af63d242c8b98344676b0. Just rebased again and simplified double ::g_ prefixes

  48. MarcoFalke commented at 1:03 PM on May 15, 2020: member

    Code review ACK 9f0a6f3. Just rebased again and simplified double ::g_ prefixes

    The commit you reviewed is not signed by me, nor does it include my proof of work. I declare this review invalid.

  49. ryanofsky commented at 1:53 PM on May 15, 2020: contributor

    Code review ACK 9f0a6f3. Just rebased again and simplified double ::g_ prefixes

    The commit you reviewed is not signed by me, nor does it include my proof of work. I declare this review invalid.

    Maybe I just liked Ariard's commit so much I just wanted to comment about it on your PR?

  50. ryanofsky approved
  51. ryanofsky commented at 1:53 PM on May 15, 2020: contributor

    Code review ACK faa438bd09cb8c940092cc9d8151524e45fa1321. This PR is pretty good, too

  52. DrahtBot cross-referenced this on May 16, 2020 from issue RFC: Introducing AltNet, a pluggable framework for alternative transports by ariard
  53. DrahtBot cross-referenced this on May 16, 2020 from issue RFC: Introducing Watchdog, a cross-layer anomaly detection module by ariard
  54. ryanofsky requested review from jamesob on May 19, 2020
  55. jamesob approved
  56. jamesob commented at 8:39 PM on May 19, 2020: member

    Re-ACK faa438bd09cb8c940092cc9d8151524e45fa1321 (jamesob/ackr/18698.2.MarcoFalke.make_g_chainman_internal)

    Verified with diff-of-diffs that the only change other than rebasing is the &g_chainman simplification. Built locally.

    <details><summary>Show platform data</summary> <p>

    Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Werror=thread-safety-analysis -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

    </p></details>

    <details><summary>Show signature data</summary> <p>

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Re-ACK faa438bd09cb8c940092cc9d8151524e45fa1321 ([`jamesob/ackr/18698.2.MarcoFalke.make_g_chainman_internal`](https://github.com/jamesob/bitcoin/tree/ackr/18698.2.MarcoFalke.make_g_chainman_internal))

Verified with diff-of-diffs that the only change other than rebasing is the `&g_chainman` 
simplification. Build locally.

<details><summary>Show platform data</summary>
<p>

    Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic

    Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion

    Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Werror=thread-safety-analysis -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2 i

    Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

    
</p></details>
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl7EQ90ACgkQyrywS1A+
5mhnuA/9EQOujOUD26ZnjbYkG7erMCOtdg6g9KPvk3tNtTeMnbckCDj61oxTGYAq
kkpJgrnqrr3Jcer7Znz423BNKqWruakfdNN+gZyZY5WHLsB3A4e5j5bGbu4c1G8f
kXU02uP+YcNj9hqd1zpz6Op1HEaju9LWEeDwnLR7ER6enxC3+tAGhyUY37IQ90eD
xR7WaES5IWnQL2ghvxXeTC1ZI97kwRcGfyrNso/VjR8se8IumLq2pToSMFYC7P0b
ksgZ+uMNHkAQvamvKS5JXhMrIdO0XXGwnmJgb7ZifB8q4S1j2VNBamwZb2wDrGX9
Skro52HsVxuC+Y4fTMcDZO8fYUGBzSJKN0r33qwzFH0/p38wHplr4bFKVRmXdtM/
H0Xnmf6fJNil2wRCz5ZQsjz7t/N0jFdPa1ffLxygCevS3+vR01gR1RknriIppoSh
wAB4OfFvKw6UHAEQoJ3SpTkEI/Px9PcfJ0GbJgugH+3bgqme/eGRU2lowVkPvG4s
uRMKQsi8pscCA65L45hL0I2lC5RJx3fX5LrJxbXPD6NW85ALx0y3Bb3Nsi30fQh/
wWfvvuXetUDcMyvzW5cOxQIdt8anWsu0X046W3U+74KgtEzhZiwJiTZjIHL6I4Z/
CbYR9nX3qhXiO7/h7yiJRJSV6TDQI5rVgZUrvGYNZriyQo3s8/s=
=hqDd
-----END PGP SIGNATURE-----

    </p></details>

  57. DrahtBot added the label Needs rebase on May 21, 2020
  58. node: Add chainman alias for g_chainman fa7b626d7a
  59. net: Pass chainman into PeerLogicValidation fa05fdf0f1
  60. validation: Make LoadBlockIndex() a member of ChainstateManager fa84b1cd84
  61. validation: Make PruneOneBlockFile() a member of ChainstateManager fa24d49098
  62. validation: Make ProcessNewBlock*() members of ChainstateManager fa1d97b256
  63. validation: Mark g_chainman DEPRECATED fab6b9d18f
  64. MarcoFalke force-pushed on May 21, 2020
  65. DrahtBot removed the label Needs rebase on May 21, 2020
  66. ryanofsky approved
  67. ryanofsky commented at 5:40 PM on May 22, 2020: contributor

    Code review ACK fab6b9d18fd48bbbd1939b1173723bc04c5824b5. Had to be rebased but still looks good

  68. MarcoFalke commented at 5:56 PM on May 22, 2020: member

    The rebases are trivial and I don't mind them, but letting this sit in the rebase-ping-pong doesn't make the changes any better over time. Is there anything left to be done here before merge?

  69. ryanofsky commented at 6:46 PM on May 22, 2020: contributor

    Is there anything left to be done here before merge?

    Not that I can think of, this looks good to me

  70. DrahtBot cross-referenced this on May 22, 2020 from issue refactor: replace CNode pointers by references within net_processing.{h,cpp} by theStack
  71. MarcoFalke merged this on May 23, 2020
  72. MarcoFalke closed this on May 23, 2020
  73. MarcoFalke deleted the branch on May 23, 2020
  74. jasonbcox referenced this in commit 2b2c1f2e1c on Dec 1, 2020
  75. jasonbcox referenced this in commit aa8e274591 on Dec 1, 2020
  76. jasonbcox referenced this in commit 464af723ff on Dec 1, 2020
  77. deadalnix referenced this in commit 2ba290c2d2 on Dec 1, 2020
  78. jasonbcox referenced this in commit c2fa271b72 on Dec 1, 2020
  79. jasonbcox referenced this in commit 57cc486f0e on Dec 1, 2020
  80. jnewbery cross-referenced this on Oct 4, 2021 from issue Add `ChainstateManager::ProcessTransaction` by jnewbery
  81. MarcoFalke referenced this in commit 38b2a0a3f9 on Nov 10, 2021
  82. bitcoin locked this on Feb 15, 2022
Contributors
MarcoFalkeDrahtBotpracticalswiftryanofskyjamesob
Labels
Refactoring
Linked (view graph)
#15606 assumeutxo#17737 Add ChainstateManager, remove BlockManager global#23173 Add `ChainstateManager::ProcessTransaction`
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: 2026-05-19 11:54 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me