Page MenuHomePhabricator

refactor: Replace string chain name constants with ChainTypes
Needs ReviewPublic

Authored by PiRK on Thu, Apr 24, 11:01.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

This commit effectively moves the definition of these constants
out of the chainparamsbase to their own file.

Using the ChainType enums provides better type safety compared to
passing around strings.

The commit is part of an ongoing effort to decouple the libbitcoinkernel
library from the ArgsManager and other functionality that should not be
part of the kernel library.

Backport note: the Config ctor could also take a ChainType param instead of a network string but lets do this in a separate diff

This is a partial backport of core#27491
https://github.com/bitcoin/bitcoin/pull/27491/commits/ba8fc7d788932b25864fb260ca14983aa2398c23

Depends on D17987

Test Plan

With Chronik compiled:
ninja all check-all bench-bitcoin bitcoin-fuzzers

Event Timeline

PiRK requested review of this revision.Thu, Apr 24, 11:01

It's a big diff, but the change is pretty trivial. I suggest paying particular attention to paymentserver.cpp during the review because of the raw pointers.

src/chainparamsbase.cpp
61–62

This line and other similar lines are effectively dead code, and should be replaced with assert false; in a future backport (see https://github.com/bitcoin/bitcoin/pull/27611)

Tail of the build log:

tests/test_iguana.py::test_redeem_script_input_sigchecks ##teamcity[testStarted timestamp='2025-04-24T11:27:18.859' captureStandardOutput='false' flowId='tests.test_iguana.test_redeem_script_input_sigchecks' metainfo='test_redeem_script_input_sigchecks' name='tests.test_iguana.test_redeem_script_input_sigchecks']
PASSED          [100%]##teamcity[testFinished timestamp='2025-04-24T11:27:18.916' duration='56' flowId='tests.test_iguana.test_redeem_script_input_sigchecks' name='tests.test_iguana.test_redeem_script_input_sigchecks']


============================== 20 passed in 0.67s ==============================
[200/521] Test Bitcoin utilities...
[201/521] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_tests.cpp.o
[202/521] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/options_tests.cpp.o
[203/521] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/db_tests.cpp.o
[204/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/compattests.cpp.o
[205/521] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/grasberg_tests.cpp.o
[206/521] Building CXX object src/pow/test/CMakeFiles/test-pow.dir/eda_tests.cpp.o
[207/521] Linking CXX executable src/pow/test/test-pow
[208/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/util.cpp.o
[209/521] pow: testing daa_tests
[210/521] Running utility command for check-pow-daa_tests
[211/521] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/wallet/test/wallet_tests.cpp.o
[212/521] pow: testing eda_tests
[213/521] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/message_writer_tests.cpp.o
[214/521] Running utility command for check-pow-eda_tests
[215/521] pow: testing grasberg_tests
[216/521] Running utility command for check-pow-grasberg_tests
[217/521] Building CXX object src/seeder/test/CMakeFiles/test-seeder.dir/p2p_messaging_tests.cpp.o
[218/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/bitcoinaddressvalidatortests.cpp.o
[219/521] Linking CXX executable src/seeder/test/test-seeder
[220/521] seeder: testing db_tests
[221/521] seeder: testing message_writer_tests
[222/521] Running utility command for check-seeder-db_tests
[223/521] Running utility command for check-seeder-message_writer_tests
[224/521] seeder: testing options_tests
[225/521] seeder: testing p2p_messaging_tests
[226/521] Running utility command for check-seeder-options_tests
[227/521] Running utility command for check-seeder-p2p_messaging_tests
[228/521] seeder: testing parse_name_tests
[229/521] Running utility command for check-seeder-parse_name_tests
[230/521] seeder: testing write_name_tests
[231/521] Running utility command for check-seeder-write_name_tests
[232/521] Running seeder test suite
PASSED: seeder test suite
[233/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_bitcoin-qt_autogen/mocs_compilation.cpp.o
[234/521] pow: testing aserti32d_tests
[235/521] Running utility command for check-pow-aserti32d_tests
[236/521] Running pow test suite
PASSED: pow test suite
[237/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/guiutiltests.cpp.o
[238/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/uritests.cpp.o
[239/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/optiontests.cpp.o
[240/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/apptests.cpp.o
[241/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/rpcnestedtests.cpp.o
[242/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/test_main.cpp.o
[243/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/paymentservertests.cpp.o
[244/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/__/__/wallet/test/wallet_test_fixture.cpp.o
[245/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/addressbooktests.cpp.o
[246/521] Building CXX object src/qt/test/CMakeFiles/test_bitcoin-qt.dir/wallettests.cpp.o
[247/521] Linking CXX executable src/qt/test/test_bitcoin-qt
[248/521] bitcoin-qt: testing test_bitcoin-qt
[249/521] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
Fabien requested changes to this revision.Thu, Apr 24, 12:38
Fabien added a subscriber: Fabien.

clang-tidy is not happy

This revision now requires changes to proceed.Thu, Apr 24, 12:38

Tail of the build log:

  hdwallet.ts                  |       0 |        0 |       0 |       0 | 12-176                    
  hmac.ts                      |       0 |        0 |       0 |       0 | 16-73                     
  index.ts                     |       0 |        0 |       0 |       0 |                           
  indexBrowser.ts              |       0 |        0 |       0 |       0 |                           
  indexNodeJs.ts               |       0 |        0 |       0 |       0 |                           
  initBrowser.ts               |       0 |      100 |       0 |       0 | 11-17                     
  initNodeJs.ts                |   54.54 |      100 |     100 |      80 | 10                        
  messages.ts                  |       0 |        0 |       0 |       0 | 18-123                    
  mnemonic.ts                  |       0 |        0 |       0 |       0 | 9-144                     
  op.ts                        |   20.13 |    23.33 |   36.36 |   39.47 | ...09,111,119-124,135-163 
  opcode.ts                    |    50.2 |    83.33 |     100 |     100 | 1                         
  pbkdf2.ts                    |       0 |      100 |       0 |       0 | 17-51                     
  script.ts                    |   28.44 |    20.58 |   29.03 |   50.87 | ...33-144,150,160,182-193 
  sigHashType.ts               |      40 |       25 |   46.15 |   78.94 | 26-38                     
  tx.ts                        |   47.25 |    45.23 |   47.61 |   87.23 | 110,114,123-125,144       
  txBuilder.ts                 |   40.74 |    33.92 |   54.54 |   80.43 | ...62,181-186,191,261-265 
  unsignedTx.ts                |   25.27 |       16 |   30.76 |   46.15 | ...12,320,326-329,345,357 
 src/address                   |   11.35 |    15.15 |    5.12 |   22.41 |                           
  address.ts                   |   10.95 |    11.36 |    3.22 |   21.05 | ...39-240,255-256,266-344 
  legacyaddr.ts                |   12.04 |    22.72 |    12.5 |      25 | ...9,23-38,70-111,124-128 
 src/ffi                       |   14.37 |    12.87 |    8.25 |   14.54 |                           
  ecash_lib_wasm_bg_browser.js |       0 |      100 |     100 |       0 | 1                         
  ecash_lib_wasm_browser.js    |       0 |        0 |       0 |       0 | 3-681                     
  ecash_lib_wasm_nodejs.js     |    29.9 |    36.11 |   17.64 |   30.09 | ...19,526-591,597-598,602 
 src/io                        |   29.72 |    41.17 |   37.87 |   57.44 |                           
  bytes.ts                     |    7.04 |       60 |   10.52 |   13.88 | 12,23-83                  
  hex.ts                       |   41.55 |       50 |   44.44 |   82.35 | 41-45,50,58               
  int.ts                       |       0 |        0 |       0 |       0 |                           
  str.ts                       |   46.15 |    83.33 |      40 |   85.71 | 15                        
  varsize.ts                   |   16.32 |    21.05 |      40 |      32 | 14-24,40-47               
  writer.ts                    |       0 |        0 |       0 |       0 |                           
  writerbytes.ts               |   42.62 |    40.62 |   53.33 |   83.87 | 34,44,54,64,80            
  writerlength.ts              |   53.33 |    83.33 |   53.84 |     100 | 1                         
 src/payment                   |       0 |        0 |       0 |       0 |                           
  asn1.ts                      |       0 |        0 |       0 |       0 | 77-439                    
  index.ts                     |       0 |        0 |       0 |       0 |                           
  x509.ts                      |       0 |      100 |     100 |       0 | 5-15                      
 src/test                      |   46.05 |    37.68 |      50 |   89.04 |                           
  testRunner.ts                |   46.05 |    37.68 |      50 |   89.04 | 66-68,81-82,105,114,157   
 src/token                     |   31.19 |    24.63 |   34.56 |   46.42 |                           
  alp.parse.ts                 |       0 |        0 |       0 |       0 | 102-256                   
  alp.ts                       |   42.68 |    53.12 |   43.47 |   83.33 | 119-132,151               
  common.ts                    |      52 |    83.33 |     100 |     100 | 1                         
  empp.ts                      |   52.17 |       60 |   57.14 |   91.66 | 12                        
  slp.parse.ts                 |       0 |        0 |       0 |       0 | 128-382                   
  slp.ts                       |   47.16 |    33.82 |      52 |   90.36 | ...82,188,196,199,218,223 
-------------------------------|---------|----------|---------|---------|---------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='879']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='4206']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='263']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='1193']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='144']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='692']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='858']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='3057']
##teamcity[blockClosed name='Code Coverage Summary']
mv: cannot stat 'test_results/ecash-lib-integration-tests-junit.xml': No such file or directory
Build ecash-lib-integration-tests failed with exit code 1
Fabien requested changes to this revision.Fri, Apr 25, 09:30

Please rebase to fix CI

This revision now requires changes to proceed.Fri, Apr 25, 09:30
Fabien requested changes to this revision.Fri, Apr 25, 14:17
Fabien added inline comments.
src/chainparamsbase.h
8 ↗(On Diff #53688)

You can forward declare ChainType instead. This deduplicates the include from the .h and .cpp

src/common/args.cpp
146 ↗(On Diff #53688)

Missing the header ?

src/networks/abc/checkpoints.cpp
149 ↗(On Diff #53688)

missing header

src/pow/test/aserti32d_tests.cpp
60 ↗(On Diff #53688)

ditto

src/pow/test/daa_tests.cpp
30 ↗(On Diff #53688)

ditto

src/qt/paymentserver.cpp
101 ↗(On Diff #53688)

header is missing

126 ↗(On Diff #53688)

not this diff fault (the change looks ok) but this code is a piece of crap. We should update this to use optional and maybe simplify the logic

src/qt/test/apptests.cpp
81 ↗(On Diff #53688)

missing header

src/test/blockmanager_tests.cpp
23 ↗(On Diff #53688)

This is a change in behavior, same on the line below (but less of an issue). Is that deliberate ?

src/test/fuzz/descriptor_parse.cpp
13 ↗(On Diff #53688)

that's not regtest :)

This revision now requires changes to proceed.Fri, Apr 25, 14:17
src/common/args.cpp
146 ↗(On Diff #53688)

This one was added in D17987

src/test/blockmanager_tests.cpp
23 ↗(On Diff #53688)

That's a mistake, we must be missing an intermediate backport

23 ↗(On Diff #53688)

That one is a bit weird. Core introduced this test with CreateChainParams(ArgsManager{}, ChainType::MAIN), we introduced it with CreateChainParams(CBaseChainParams::MAIN) and then later added the *m_node.args in D16114. Out of sequence backport, different end result.
Apparently it does not matter for the purpose of this test, so I'll revert the unrelated change.

src/test/fuzz/descriptor_parse.cpp
13 ↗(On Diff #53688)

Looks like we are missing core#20300. Let's revert, as this change is unrelated (the proper chain depends on the corpus)

missing headers ++, unrelated changes --, forward declaration in chainparamsbase.h

Tail of the build log:

      groups: "a"
      data: "abc"
    }
  }
}
time_first_seen: 1745611116
size: 178
])
2025-04-25T19:58:37.075000Z TestFramework (INFO): Stopping nodes
2025-04-25T19:58:37.227000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/chronik-client-integration-tests/test/tmp/test_runner_₿₵_🏃_20250425_195835_38085/setup_scripts/chronik-client_plugins_0
2025-04-25T19:58:37.228000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/chronik-client-integration-tests/test/tmp/test_runner_₿₵_🏃_20250425_195835_38085/setup_scripts/chronik-client_plugins_0/test_framework.log
2025-04-25T19:58:37.228000Z TestFramework (ERROR): 
2025-04-25T19:58:37.228000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/chronik-client-integration-tests/test/tmp/test_runner_₿₵_🏃_20250425_195835_38085/setup_scripts/chronik-client_plugins_0' to consolidate all logs
2025-04-25T19:58:37.228000Z TestFramework (ERROR): 
2025-04-25T19:58:37.228000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2025-04-25T19:58:37.228000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2025-04-25T19:58:37.229000Z TestFramework (ERROR): 
Running Unit Tests for Test Framework Modules
setup_scripts/chronik-client_plugins.py started
setup_scripts/chronik-client_plugins.py failed, Duration: 2 s

stdout:

stderr:


TEST                                    | STATUS    | DURATION

setup_scripts/chronik-client_plugins.py | ✖ Failed  | 2 s

ALL                                     | ✖ Failed  | 2 s (accumulated) 
Runtime: 2 s

Test runner for chronik-client_plugins completed with code 1
----------------------|---------|----------|---------|---------|------------------------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                  
----------------------|---------|----------|---------|---------|------------------------------------
All files             |   30.96 |    12.36 |   27.88 |   30.96 |                                    
 chronik-client       |     100 |      100 |     100 |     100 |                                    
  index.ts            |     100 |      100 |     100 |     100 |                                    
 chronik-client/proto |   23.53 |     9.16 |   17.05 |   23.74 |                                    
  chronik.ts          |   23.53 |     9.16 |   17.05 |   23.74 | ...4,6520-6523,6529-6571,6607-6616 
 chronik-client/src   |   63.52 |    48.82 |   59.86 |    63.1 |                                    
  ChronikClient.ts    |   55.77 |    47.54 |   56.63 |   56.18 | ...2,1463-1471,1479-1544,1552-1557 
  failoverProxy.ts    |   78.89 |    58.06 |   69.23 |    78.3 | ...282-285,288,297,304,308,313,317 
  hex.ts              |   89.47 |       50 |      75 |   87.87 | 58,66-68                           
  validation.ts       |    75.6 |       40 |      75 |   72.97 | 17,21,33,38-49,62-63               
----------------------|---------|----------|---------|---------|------------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='995']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='3213']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='260']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='2102']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='162']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='581']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='983']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='3175']
##teamcity[blockClosed name='Code Coverage Summary']
Build chronik-client-integration-tests failed with exit code 1