Page MenuHomePhabricator

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

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

Details

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

Event Timeline

PiRK requested review of this revision.May 22 2024, 11:46

nit: alphabetical sorting in CMakeLists.txt

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

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 edited the summary of this revision. (Show Details)

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

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

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

Looking into the test failure

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

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).

src/common/args.cpp
789 ↗(On Diff #47891)

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