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: 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:

    • #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. MarcoFalke force-pushed on Apr 24, 2020
  7. MarcoFalke commented at 7:09 pm on April 24, 2020: member
    Rebased due to conflict in tests
  8. 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.
  9. 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
  10. ryanofsky approved
  11. ryanofsky commented at 7:28 am on May 1, 2020: member
    Code review ACK fa24ea2e67d9812a7b57f9820996a8695a8fef37. Looks all good. Ignore my suggestions below. Would be good to have @jamesob concept ACK
  12. DrahtBot added the label Needs rebase on May 1, 2020
  13. MarcoFalke force-pushed on May 5, 2020
  14. DrahtBot removed the label Needs rebase on May 5, 2020
  15. jamesob commented at 8:25 pm on May 6, 2020: member
    Concept ACK! Reviewing in detail now.
  16. jamesob approved
  17. 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.

    0Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic
1
2Configured 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
3
4Compiled 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
5
6Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK fa5f7c0b47065476a9ba0a716d9a35c2d0dde2df ([`jamesob/ackr/18698.1.MarcoFalke.make_g_chainman_internal`](https://github.com/jamesob/bitcoin/tree/ackr/18698.1.MarcoFalke.make_g_chainman_internal))
 4
 5Built locally. Some commits most easily reviewed with 
 6`--color-moved=dimmed_zebra --ignore-space-change`.
 7
 8This is a big step in the right direction. Ultimately it would be great (as
 9[@ryanofsky](/bitcoin-bitcoin/contributor/ryanofsky/) alluded to) to be able to remove the `g_chainman` symbol completely,
10but that will require some bigger changes, like parameterizing `ChainstateActive()`
11by a `node`.
12
13This is an obvious improvement IMO if there are fuzzing benefits.
14
15<details><summary>Show platform data</summary>
16<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)

     0
 1</p></details>
 2-----BEGIN PGP SIGNATURE-----
 3
 4iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl60IUYACgkQyrywS1A+
 55miwVg/+Plvj8qNPAp5ajpXzpjhxp3aJ3T+BEQL3mssNc7isL9loO6hjpajnuD+X
 6M8+BreXXBScqE5SurAA88y3hy6YWW00aiktA4EtTXAy5x2R0MOeL71oQ81VCNVV2
 7ev85rf8TWPD4obiPL8NtZ0AP48s3ttn4rBgFLDXyWR3i097qmDVqqHZcszw+8z/0
 8R2jT5ocf8XLp+MKIXKrw6h03JAN7LxOssBI1BIX0Da5A1Zp291FxY10W883hUE/d
 9AR75gwqjTXJPDuoIz4KHtmsEd/6bv/rvk4PI68ktZc0rUxt9oJQxFtaftrmUK5SY
10MezBTkX3s6e/zPRM0v2ma8aEQd7l01yvvBoYOFaQdyxtFD0CzoxToE2m9HEgEcaX
11dojkVnVdwelT+SG15nwPOMYAs8G9YPW3c6RcS+KD8TlDIk2q5BWoVcGIspU50eZv
12bHtnOyJ6Zcyw3AHA5HwfM/0Ft7j0I/uKNuK2qtHkYc0JfNNeMh3I+Hw1R2J1bD0u
13jj/idtoZ1RyU9niBWun/7hUIXByeBXJTzR4dM8adwfTtR30T/5PX90wfsndn56DM
14RA2LA47NvKQb1h9cZyPyzWic1zQGzBCA/Hf1GCccTFC9hNS1kuax/U7eJYMDLEqR
15QCU7JU9b673kvR2e54rsfpI+xYf78izdmnXwJU5xHPVX8k6q3lo=
16=up7+
17-----END PGP SIGNATURE-----
  18. DrahtBot added the label Needs rebase on May 12, 2020
  19. ryanofsky approved
  20. ryanofsky commented at 7:10 pm on May 13, 2020: member
    Code review ACK fa5f7c0b47065476a9ba0a716d9a35c2d0dde2df. Just rebased since last review (and needs another rebase)
  21. MarcoFalke force-pushed on May 14, 2020
  22. MarcoFalke force-pushed on May 14, 2020
  23. DrahtBot removed the label Needs rebase on May 14, 2020
  24. ryanofsky approved
  25. ryanofsky commented at 10:01 pm on May 14, 2020: member
    Code review ACK 9f0a6f36b5ccf08e054af63d242c8b98344676b0. Just rebased again and simplified double ::g_ prefixes
  26. 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.

  27. ryanofsky commented at 1:53 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.

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

  28. ryanofsky approved
  29. ryanofsky commented at 1:53 pm on May 15, 2020: member
    Code review ACK faa438bd09cb8c940092cc9d8151524e45fa1321. This PR is pretty good, too
  30. ryanofsky requested review from jamesob on May 19, 2020
  31. jamesob approved
  32. 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.

    0Tested on Linux-5.3.0-51-generic-x86_64-with-Ubuntu-18.04-bionic
1
2Configured 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
3
4Compiled 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
5
6Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

    0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3Re-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))
4
5Verified with diff-of-diffs that the only change other than rebasing is the `&g_chainman` 
6simplification. Build locally.
7
8<details><summary>Show platform data</summary>
9<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)

     0
 1</p></details>
 2-----BEGIN PGP SIGNATURE-----
 3
 4iQIzBAEBCgAdFiEEpm2+LMqPsWj2rAwWyrywS1A+5mgFAl7EQ90ACgkQyrywS1A+
 55mhnuA/9EQOujOUD26ZnjbYkG7erMCOtdg6g9KPvk3tNtTeMnbckCDj61oxTGYAq
 6kkpJgrnqrr3Jcer7Znz423BNKqWruakfdNN+gZyZY5WHLsB3A4e5j5bGbu4c1G8f
 7kXU02uP+YcNj9hqd1zpz6Op1HEaju9LWEeDwnLR7ER6enxC3+tAGhyUY37IQ90eD
 8xR7WaES5IWnQL2ghvxXeTC1ZI97kwRcGfyrNso/VjR8se8IumLq2pToSMFYC7P0b
 9ksgZ+uMNHkAQvamvKS5JXhMrIdO0XXGwnmJgb7ZifB8q4S1j2VNBamwZb2wDrGX9
10Skro52HsVxuC+Y4fTMcDZO8fYUGBzSJKN0r33qwzFH0/p38wHplr4bFKVRmXdtM/
11H0Xnmf6fJNil2wRCz5ZQsjz7t/N0jFdPa1ffLxygCevS3+vR01gR1RknriIppoSh
12wAB4OfFvKw6UHAEQoJ3SpTkEI/Px9PcfJ0GbJgugH+3bgqme/eGRU2lowVkPvG4s
13uRMKQsi8pscCA65L45hL0I2lC5RJx3fX5LrJxbXPD6NW85ALx0y3Bb3Nsi30fQh/
14wWfvvuXetUDcMyvzW5cOxQIdt8anWsu0X046W3U+74KgtEzhZiwJiTZjIHL6I4Z/
15CbYR9nX3qhXiO7/h7yiJRJSV6TDQI5rVgZUrvGYNZriyQo3s8/s=
16=hqDd
17-----END PGP SIGNATURE-----
  33. DrahtBot added the label Needs rebase on May 21, 2020
  34. node: Add chainman alias for g_chainman fa7b626d7a
  35. net: Pass chainman into PeerLogicValidation fa05fdf0f1
  36. validation: Make LoadBlockIndex() a member of ChainstateManager fa84b1cd84
  37. validation: Make PruneOneBlockFile() a member of ChainstateManager fa24d49098
  38. validation: Make ProcessNewBlock*() members of ChainstateManager fa1d97b256
  39. validation: Mark g_chainman DEPRECATED fab6b9d18f
  40. MarcoFalke force-pushed on May 21, 2020
  41. DrahtBot removed the label Needs rebase on May 21, 2020
  42. ryanofsky approved
  43. ryanofsky commented at 5:40 pm on May 22, 2020: member
    Code review ACK fab6b9d18fd48bbbd1939b1173723bc04c5824b5. Had to be rebased but still looks good
  44. 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?
  45. ryanofsky commented at 6:46 pm on May 22, 2020: member

    Is there anything left to be done here before merge?

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

  46. MarcoFalke merged this on May 23, 2020
  47. MarcoFalke closed this on May 23, 2020
  48. MarcoFalke deleted the branch on May 23, 2020
  49. jasonbcox referenced this in commit 2b2c1f2e1c on Dec 1, 2020
  50. jasonbcox referenced this in commit aa8e274591 on Dec 1, 2020
  51. jasonbcox referenced this in commit 464af723ff on Dec 1, 2020
  52. deadalnix referenced this in commit 2ba290c2d2 on Dec 1, 2020
  53. jasonbcox referenced this in commit c2fa271b72 on Dec 1, 2020
  54. jasonbcox referenced this in commit 57cc486f0e on Dec 1, 2020
  55. MarcoFalke referenced this in commit 38b2a0a3f9 on Nov 10, 2021
  56. DrahtBot locked this on Feb 15, 2022


MarcoFalke DrahtBot practicalswift ryanofsky jamesob

Labels
Refactoring

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-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me