Page MenuHomePhabricator
Differential D16202

move-only: Extract common/args and common/config.cpp from util/system
ClosedPublic
Actions

Authored by PiRK on May 22 2024, 11:46.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCecada1e39579: move-only: Extract common/args and common/config.cpp from util/system
Summary

This is an extraction of ArgsManager related functions from util/system
into their own common file.

Config file related functions are moved to common/configfile.cpp.

The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.

This is a backport of core#27419
Depends on D16203

Note that due to missing backports, I had to make CheckValid and InterpretOption non-static and add them to args.h so that they are available both for args.cpp and configfile.cpp. These two static functions will go away when we backport core#22766

Test Plan

ninja all check-all bench-bitcoin bitcoin-fuzzers

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.May 22 2024, 11:46
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 22 2024, 11:46
PiRK requested review of this revision.May 22 2024, 11:46
teamcity edited the summary of this revision. (Show Details)May 22 2024, 11:46
PiRK updated this revision to Diff 47887.May 22 2024, 11:52

nit: alphabetical sorting in CMakeLists.txt

Harbormaster completed remote builds in B29071: Diff 47884.May 22 2024, 12:18
Harbormaster completed remote builds in B29074: Diff 47887.May 22 2024, 12:44
PiRK added inline comments.May 22 2024, 14:22
src/chainparams.cpp
30 ↗(On Diff #47887)

The linter wants to remove this line (introduced in D16124)

Fabien requested changes to this revision.May 22 2024, 14:31
Fabien added a subscriber: Fabien.

There seem to be a lot of include changes that are not the 2 impacted files and could be split apart. This is a big diff and this makes the review much more difficult

src/avalanche/proofbuilder.cpp
8 ↗(On Diff #47887)

Did you forget to replace it or is it simply not needed ? If the latter you should make it its own diff

src/banman.cpp
8 ↗(On Diff #47887)

Unrelated ?

src/bitcoin-cli.cpp
22 ↗(On Diff #47887)

dito ?

This revision now requires changes to proceed.May 22 2024, 14:31
PiRK added a comment.May 22 2024, 14:40

Fixing the includes in args.h and system.h makes these missing includes (mostly logging.h) necessary. I can add them in a previous diff to limit this diff to args.h and system.h

PiRK mentioned this in D16203: add missing includes and remove unneeded ones.May 22 2024, 15:03
PiRK updated this revision to Diff 47891.May 22 2024, 15:11
PiRK edited the summary of this revision. (Show Details)
PiRK added a parent revision: D16203: add missing includes and remove unneeded ones.

rebase on D16203 (removes include changes that are not directly related)

PiRK added inline comments.May 22 2024, 15:16
src/policy/block/stakingrewards.cpp
10 ↗(On Diff #47891)

previously system.h was somehow included which made gArgs available.
Now the explicit include is needed because common/args.h is not included indirectly

Harbormaster completed remote builds in B29076: Diff 47891.May 22 2024, 15:49
teamcity added a comment.May 22 2024, 15:49
Build Bitcoin ABC Diffs / Diff Testing (chronik-client-integration-tests) failed.

Failed tests logs:

====== Test expected websocket behavior of chronik-client: After a block is avalanche finalized.Test expected websocket behavior of chronik-client After a block is avalanche finalized ======
AssertionError: expected undefined to deeply equal { type: 'Block', …(3) }
    at /work/modules/chronik-client/test/integration/websocket.ts:336:43
    at Generator.next (<anonymous>)
    at fulfilled (test/integration/websocket.ts:31:58)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual:  failed to generate Mocha diff
====== Test expected websocket behavior of chronik-client: After some txs have been broadcast.Test expected websocket behavior of chronik-client After some txs have been broadcast ======
AssertionError: expected { type: 'Block', …(3) } to deeply equal { type: 'Tx', …(2) }
    at /work/modules/chronik-client/test/integration/websocket.ts:348:41
    at Generator.next (<anonymous>)
    at fulfilled (test/integration/websocket.ts:31:58)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       {
      -  "blockHash": "67c9f9e49f00516a4a7cba209e962c1571f11fb98e9127ae6d73fafb7f0bf12b"
      -  "blockHeight": 217
      -  "msgType": "BLK_FINALIZED"
      -  "type": "Block"
      +  "msgType": "TX_ADDED_TO_MEMPOOL"
      +  "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
      +  "type": "Tx"
       }
====== Test expected websocket behavior of chronik-client: After a block is mined.Test expected websocket behavior of chronik-client After a block is mined ======
AssertionError: expected [ { type: 'Tx', …(2) }, …(3) ] to have the same members as [ { type: 'Tx', …(2) }, …(3) ]
    at /work/modules/chronik-client/test/integration/websocket.ts:404:43
    at Generator.next (<anonymous>)
    at fulfilled (test/integration/websocket.ts:31:58)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       [
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "1e4f143e336f5dcbb2e3483f662121bf81301f86da124fed44f59ad7fe4108b4"
      +    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "0458181e236fecb292f23b02b0551b8984ad7afd72b30c57af9fe582eb1e60a1"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "054375a8f5c28f80a2c4bbc8408b93f8cad5714c6fa683686fa3b6f6b6340531"
      +    "txid": ""
           "type": "Tx"
         }
       ]
====== Test expected websocket behavior of chronik-client: After this block is parked.Test expected websocket behavior of chronik-client After this block is parked ======
AssertionError: expected { type: 'Tx', …(2) } to deeply equal { type: 'Tx', …(2) }
    at /work/modules/chronik-client/test/integration/websocket.ts:426:39
    at Generator.next (<anonymous>)
    at /work/modules/chronik-client/test/integration/websocket.ts:34:71
    at new Promise (<anonymous>)
    at __awaiter (test/integration/websocket.ts:30:12)
    at Context.<anonymous> (test/integration/websocket.ts:409:49)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       {
         "msgType": "TX_ADDED_TO_MEMPOOL"
      -  "txid": "0458181e236fecb292f23b02b0551b8984ad7afd72b30c57af9fe582eb1e60a1"
      +  "txid": ""
         "type": "Tx"
       }
====== Test expected websocket behavior of chronik-client: After this block is unparked.Test expected websocket behavior of chronik-client After this block is unparked ======
AssertionError: expected [ { type: 'Tx', …(2) }, …(3) ] to have the same members as [ { type: 'Tx', …(2) }, …(3) ]
    at /work/modules/chronik-client/test/integration/websocket.ts:464:43
    at Generator.next (<anonymous>)
    at /work/modules/chronik-client/test/integration/websocket.ts:34:71
    at new Promise (<anonymous>)
    at __awaiter (test/integration/websocket.ts:30:12)
    at Context.<anonymous> (test/integration/websocket.ts:436:51)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       [
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "1e4f143e336f5dcbb2e3483f662121bf81301f86da124fed44f59ad7fe4108b4"
      +    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "0458181e236fecb292f23b02b0551b8984ad7afd72b30c57af9fe582eb1e60a1"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "054375a8f5c28f80a2c4bbc8408b93f8cad5714c6fa683686fa3b6f6b6340531"
      +    "txid": ""
           "type": "Tx"
         }
       ]
====== Test expected websocket behavior of chronik-client: After this block is invalidated.Test expected websocket behavior of chronik-client After this block is invalidated ======
AssertionError: expected { type: 'Tx', …(2) } to deeply equal { type: 'Tx', …(2) }
    at /work/modules/chronik-client/test/integration/websocket.ts:486:39
    at Generator.next (<anonymous>)
    at /work/modules/chronik-client/test/integration/websocket.ts:34:71
    at new Promise (<anonymous>)
    at __awaiter (test/integration/websocket.ts:30:12)
    at Context.<anonymous> (test/integration/websocket.ts:469:54)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       {
         "msgType": "TX_ADDED_TO_MEMPOOL"
      -  "txid": "0458181e236fecb292f23b02b0551b8984ad7afd72b30c57af9fe582eb1e60a1"
      +  "txid": ""
         "type": "Tx"
       }
====== Test expected websocket behavior of chronik-client: After this block is reconsidered.Test expected websocket behavior of chronik-client After this block is reconsidered ======
AssertionError: expected [ { type: 'Tx', …(2) }, …(3) ] to have the same members as [ { type: 'Tx', …(2) }, …(3) ]
    at /work/modules/chronik-client/test/integration/websocket.ts:524:43
    at Generator.next (<anonymous>)
    at /work/modules/chronik-client/test/integration/websocket.ts:34:71
    at new Promise (<anonymous>)
    at __awaiter (test/integration/websocket.ts:30:12)
    at Context.<anonymous> (test/integration/websocket.ts:496:55)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       [
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "1e4f143e336f5dcbb2e3483f662121bf81301f86da124fed44f59ad7fe4108b4"
      +    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "0458181e236fecb292f23b02b0551b8984ad7afd72b30c57af9fe582eb1e60a1"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_CONFIRMED"
      -    "txid": "054375a8f5c28f80a2c4bbc8408b93f8cad5714c6fa683686fa3b6f6b6340531"
      +    "txid": ""
           "type": "Tx"
         }
       ]
====== Test expected websocket behavior of chronik-client: After this block is finalized by Avalanche.Test expected websocket behavior of chronik-client After this block is finalized by Avalanche ======
AssertionError: expected [ { type: 'Tx', …(2) }, …(3) ] to have the same members as [ { type: 'Tx', …(2) }, …(3) ]
    at /work/modules/chronik-client/test/integration/websocket.ts:553:43
    at Generator.next (<anonymous>)
    at /work/modules/chronik-client/test/integration/websocket.ts:34:71
    at new Promise (<anonymous>)
    at __awaiter (test/integration/websocket.ts:30:12)
    at Context.<anonymous> (test/integration/websocket.ts:529:65)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

      + expected - actual

       [
         {
           "msgType": "TX_FINALIZED"
      -    "txid": "1e4f143e336f5dcbb2e3483f662121bf81301f86da124fed44f59ad7fe4108b4"
      +    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
           "type": "Tx"
         }
         {
           "msgType": "TX_FINALIZED"
      -    "txid": "a6e12d72e220c7960db6c501cd6dba8f5fcbb01255b50c405de782675ec44012"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_FINALIZED"
      -    "txid": "0458181e236fecb292f23b02b0551b8984ad7afd72b30c57af9fe582eb1e60a1"
      +    "txid": ""
           "type": "Tx"
         }
         {
           "msgType": "TX_FINALIZED"
      -    "txid": "054375a8f5c28f80a2c4bbc8408b93f8cad5714c6fa683686fa3b6f6b6340531"
      +    "txid": ""
           "type": "Tx"
         }
       ]

Each failure log is accessible here:
Test expected websocket behavior of chronik-client: After a block is avalanche finalized.Test expected websocket behavior of chronik-client After a block is avalanche finalized
Test expected websocket behavior of chronik-client: After some txs have been broadcast.Test expected websocket behavior of chronik-client After some txs have been broadcast
Test expected websocket behavior of chronik-client: After a block is mined.Test expected websocket behavior of chronik-client After a block is mined
Test expected websocket behavior of chronik-client: After this block is parked.Test expected websocket behavior of chronik-client After this block is parked
Test expected websocket behavior of chronik-client: After this block is unparked.Test expected websocket behavior of chronik-client After this block is unparked
Test expected websocket behavior of chronik-client: After this block is invalidated.Test expected websocket behavior of chronik-client After this block is invalidated
Test expected websocket behavior of chronik-client: After this block is reconsidered.Test expected websocket behavior of chronik-client After this block is reconsidered
Test expected websocket behavior of chronik-client: After this block is finalized by Avalanche.Test expected websocket behavior of chronik-client After this block is finalized by Avalanche

PiRK added a comment.May 23 2024, 07:28

Looking into the test failure

Fabien accepted this revision.May 23 2024, 07:37
Fabien added inline comments.
src/common/args.cpp
789 ↗(On Diff #47891)

It's weird it doesn't belong to config.cpp, is this an oversight ?

src/wallet/load.cpp
10 ↗(On Diff #47891)

This belongs to the parent diff

This revision is now accepted and ready to land.May 23 2024, 07:37
PiRK added a comment.May 23 2024, 07:37

It is not related to this diff.
The first message in websocket.ts (line 336) seems to have timed out, and after that all tests fail because they receive the previous message (everything is shifted by 1 message).

PiRK added inline comments.May 23 2024, 07:42
src/common/args.cpp
789 ↗(On Diff #47891)

I agree it is weird, but I checked and it matches the source material

Closed by commit rABCecada1e39579: move-only: Extract common/args and common/config.cpp from util/system (authored by PiRK). · Explain WhyMay 23 2024, 07:58
This revision was automatically updated to reflect the committed changes.
PiRK mentioned this in rABC4c05a04a597e: add missing includes and remove unneeded ones.
PiRK added a commit: rABCecada1e39579: move-only: Extract common/args and common/config.cpp from util/system.

Large Diff

This large diff affects 104 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents
Changeset List

PathSize
arcanist/
linter/
2 lines
chronik/
chronik-cpp/
2 lines
src/
4 lines
2 lines
avalanche/
2 lines
2 lines
1 line
2 lines
bench/
2 lines
2 lines
3 lines
1 line
3 lines
3 lines
2 lines
2 lines
common/
util/
args.h
system.h
83 lines
args.cpp
system.cpp
387 lines
common/
241 lines
consensus/
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
index/
1 line
2 lines
1 line
2 lines
1 line
init/
2 lines
2 lines
2 lines
2 lines
2 lines
node/
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
policy/
block/
2 lines
1 line
1 line
qt/
4 lines
2 lines
1 line
2 lines
2 lines
1 line
2 lines
2 lines
test/
2 lines
2 lines
2 lines
2 lines
2 lines
rpc/
1 line
1 line
3 lines
2 lines
1 line
1 line
1 line
1 line
script/
2 lines
seeder/
1 line
2 lines
2 lines
test/
2 lines
test/
2 lines
2 lines
2 lines
fuzz/
2 lines
1 line
1 line
2 lines
1 line
util/
2 lines
1 line
2 lines
2 lines
util/
460 lines
1104 lines
1 line
wallet/
1 line
2 lines
2 lines
2 lines
3 lines
rpc/
2 lines
1 line
2 lines
1 line
2 lines
2 lines
zmq/
2 lines
test/
lint/
2 lines

Diff 47917

View Options

arcanist/linter/LocaleDependenceLinter.php

View Options

chronik/chronik-cpp/chronik.cpp

View Options

src/CMakeLists.txt

View Options

src/addrdb.cpp

View Options

src/avalanche/avalanche.cpp

View Options

src/avalanche/peermanager.cpp

View Options

src/avalanche/processor.cpp

View Options

src/avalanche/proof.cpp

View Options

src/bench/bench_bitcoin.cpp

View Options

src/bitcoin-chainstate.cpp

View Options

src/bitcoin-cli.cpp

View Options

src/bitcoin-tx.cpp

View Options

src/bitcoin-wallet.cpp

View Options

src/bitcoind.cpp

View Options

src/chainparams.cpp

View Options

src/chainparamsbase.cpp

View Options

src/common/args.h

View Options

src/common/args.cpp

View Options

src/common/configfile.cpp

View Options

src/consensus/activation.cpp

View Options

src/consensus/amount.cpp

View Options

src/currencyunit.cpp

View Options

src/dnsseeds.cpp

View Options

src/dummywallet.cpp

View Options

src/httprpc.cpp

View Options

src/httpserver.cpp

View Options

src/index/base.cpp

View Options

src/index/blockfilterindex.cpp

View Options

src/index/coinstatsindex.cpp

View Options

src/index/txindex.cpp

View Options

src/init/common.cpp

View Options

src/mempool_args.cpp

View Options

src/minerfund.cpp

View Options

src/net_processing.cpp

View Options

src/node/blockmanager_args.cpp

View Options

src/node/caches.cpp

View Options

src/node/chainstatemanager_args.cpp

View Options

src/node/coins_view_args.cpp

View Options

src/node/database_args.cpp

View Options

src/node/interfaces.cpp

View Options

src/node/mempool_persist_args.cpp

View Options

src/node/miner.cpp

View Options

src/node/utxo_snapshot.cpp

View Options

src/node/validation_cache_args.cpp

View Options

src/policy/block/preconsensus.cpp

View Options

src/policy/block/stakingrewards.cpp

View Options

src/protocol.cpp

View Options

src/qt/bitcoin.cpp

View Options

src/qt/bitcoinunits.cpp

View Options

src/qt/clientmodel.cpp

View Options

src/qt/guiutil.cpp

View Options

src/qt/intro.cpp

View Options

src/qt/optionsmodel.cpp

View Options

src/qt/paymentrequestplus.cpp

View Options

src/qt/paymentserver.cpp

View Options

src/qt/test/bitcoinaddressvalidatortests.cpp

View Options

src/qt/test/uritests.cpp

View Options

src/qt/transactiondesc.cpp

View Options

src/qt/utilitydialog.cpp

View Options

src/qt/walletmodel.cpp

View Options

src/rpc/avalanche.cpp

View Options

src/rpc/blockchain.cpp

View Options

src/rpc/client.cpp

View Options

src/rpc/request.cpp

View Options

src/rpc/server.h

View Options

src/rpc/server.cpp

View Options

src/rpc/server_util.cpp

View Options

src/rpc/util.cpp

View Options

src/script/descriptor.cpp

View Options

src/seeder/bitcoin.cpp

View Options

src/seeder/main.cpp

View Options

src/seeder/options.cpp

View Options

src/seeder/test/options_tests.cpp

View Options

src/test/checkqueue_tests.cpp

View Options

src/test/denialofservice_tests.cpp

View Options

src/test/flatfile_tests.cpp

View Options

src/test/fuzz/banman.cpp

View Options

src/test/fuzz/integer.cpp

View Options

src/test/fuzz/string.cpp

View Options

src/test/fuzz/system.cpp

View Options

src/test/fuzz/transaction.cpp

View Options

src/test/util/setup_common.h

View Options

src/test/util/setup_common.cpp

View Options

src/timedata.cpp

View Options

src/torcontrol.cpp

View Options

src/util/system.h

View Options

src/util/system.cpp

View Options

src/validation.cpp

View Options

src/wallet/bdb.cpp

View Options

src/wallet/coincontrol.cpp

View Options

src/wallet/init.cpp

View Options

src/wallet/interfaces.cpp

View Options

src/wallet/load.cpp

View Options

src/wallet/rpc/backup.cpp

View Options

src/wallet/scriptpubkeyman.cpp

View Options

src/wallet/spend.cpp

View Options

src/wallet/wallet.cpp

View Options

src/wallet/walletdb.cpp

View Options

src/wallet/walletutil.cpp

View Options

src/zmq/zmqnotificationinterface.cpp

View Options

test/lint/lint-circular-dependencies.sh