feature: Use different datadirs for different signets #34566

Missing a test that verifies truncation to 16 chars for a longer challenge. The 522103ad5e... signet challenge would work. Just add an assert_node_datadir(self.nodes[4], "signet_522103ad5e0edad1") check.

Oh, right, thanks! Done in a2d755132b4

The default signet challenge string is now duplicated in chainparamsbase.cpp and kernel/chainparams.cpp (and also in the Python test framework). I'm not sure if this is actually a problem, but could this be defined as a shared constant? I'm still learning the codebase, so I'd appreciate some input for better practices here.

I had the same question, and since I’m also new, I was hoping for feedback from more experienced developers as well

I fixed this in 63cf3340748.

I added test/functional/test_framework/signet.py so test_node.py and feature_signet.py can import the constant from there.

For the C++ code, I'm now using HexStr(CChainParams::SigNet({})->GetConsensus().signet_challenge).

Thanks for making me look into it again!

Oh, this introduced a circular dependency:

A new circular dependency in the form of "chainparams -> chainparamsbase -> chainparams" appears to have been introduced.

https://github.com/bitcoin/bitcoin/actions/runs/24139839220/job/70438028501?pr=34566

Fixed in 364969087e0. I accidentally included <chainparams.h> in chainparamsbase.cpp instead of <kernel/chainparams.h>.

what is the extra_args parameter for?

This passes the signet challenge of the node to mine_block. It's required to pass -signetchallenge to the RPC. Without it, RPC authentication fails because the cookie isn't found. It will look in the default signet datadir:

$ build/test/functional/test_runner.py tool_signet_miner.py
Temporary test directory at /tmp/nix-shell-115046-4006075642/test_runner_₿_🏃_20260408_191958
Remaining jobs: [tool_signet_miner.py]
1/1 - tool_signet_miner.py failed, Duration: 0 s

stdout:
2026-04-08T17:19:58.648068Z TestFramework (INFO): PRNG seed is: 8822581970357866232
2026-04-08T17:19:58.698730Z TestFramework (INFO): Initializing test directory /tmp/nix-shell-115046-4006075642/test_runner_₿_🏃_20260408_191958/tool_signet_miner_0
2026-04-08T17:19:59.085274Z TestFramework (INFO): Signet node with single signature challenge
error: Authorization failed: Failed to read cookie file and no rpcpassword was specified. Configuration file: (/tmp/nix-shell-115046-4006075642/test_runner_₿_🏃_20260408_191958/tool_signet_miner_0/node0/bitcoin.conf)

I think it would make more sense if you just changed the name of the argument then:

def mine_block(self, node, extra_args):

Since this is how its being used (just appending args to a CLI command)

Done in 8824d95819

could be constexpr std::string_view base_data_dir = "signet";

Would it be worth it if I have to cast to std::string anyway on return?

diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp
index 8dc0296649..addd1551b0 100644
--- a/src/chainparamsbase.cpp
+++ b/src/chainparamsbase.cpp
@@ -40,10 +40,10 @@ const CBaseChainParams& BaseParams()

 std::string GetSignetDataDir()
 {
-    std::string base_data_dir = "signet";
+    constexpr std::string_view base_data_dir{"signet"};
     const std::string challenge_hex = gArgs.GetArg("-signetchallenge", "");
     if (challenge_hex.empty()) {
-        return base_data_dir;
+        return std::string(base_data_dir);
     }

     const std::vector<uint8_t> challenge_bytes = ParseHex<uint8_t>(challenge_hex);
@@ -55,9 +55,9 @@ std::string GetSignetDataDir()

     const MessageStartChars default_msg_start = CChainParams::SigNet({})->MessageStart();
     if (msg_start == default_msg_start) {
-        return base_data_dir;
+        return std::string(base_data_dir);
     }
-    return base_data_dir + "_" + HexStr(msg_start);
+    return std::string(base_data_dir) + "_" + HexStr(msg_start);
 }

 /**

CBaseChainParams expects std::string, so afaict, I have to use std::string as the return type of GetSignetDataDir().

Sorry you're right 😬 tidy didn't like this either

is challenge_hex guaranteed to be valid hex at this point? Otherwise I think this is empty but that might be ok since I'm fairly certain we'd fail later on if the challenge isnt valid.

is challenge_hex guaranteed to be valid hex at this point?

Good question! No. This line in chainparamsbase.cpp gets hit before we fail because of invalid hex here in chainparams.cpp:

https://github.com/bitcoin/bitcoin/blob/0c0f75eaaf173d31d31317ba396a6a94baac868a/src/chainparams.cpp#L36-L39

I don't see this having any undesired side effect like creating a datadir that can't be used, though, so I also think it's okay.

It just reminds me that I originally intended this functionality to be part of CChainParams instead of CBaseChainParams, but DataDir() is part of the CBaseChainParams interface. I explored conditionally adding -datadir with SoftSetArg like we do for other options, but then we would still create a "signet" folder inside it.

What if the hard-coded default signet challenge was moved from kernel/chainparams.cpp to src/signet.h as a constexpr. Then you can just compare the actual challenge provided by the user string-to-string (although probably need ToLower for case sensitivity)

  // signet.h                                                                                                                                                                                                                                                                                                   
  constexpr std::string_view SIGNET_DEFAULT_CHALLENGE{                                                                                                                                                                                                                                                                      
      "512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae"
  };                                                                                                                                                                                                                                                                                                                        

  if (challenge_hex.empty() || challenge_hex == SIGNET_DEFAULT_CHALLENGE) {
      return std::string(base_data_dir);                                                                                                                                                                                                                                                                                    
  }

I wonder if the message start method could be de-duplicated into signet.h, cpp as well ?

Done in 2fd5da15ea

I had to update src/CMakeLists.txt, see commit message

probably need ToLower for case sensitivity

I considered to add a test for this, but I think it would be too granular

edit: Okay, looking at CI, I think I need to update other CMakeLists.txt, too

Since you're using startswith here is there a risk of signet being confused with signet_XXXXXXXX ?

No, this would be caught when checking the directory name in line 111.

For example:

diff --git a/test/functional/feature_signet.py b/test/functional/feature_signet.py
index 9bc8353785..81f0246b22 100755
--- a/test/functional/feature_signet.py
+++ b/test/functional/feature_signet.py
@@ -117,7 +117,7 @@ class SignetBasicTest(BitcoinTestFramework):
             assert rpc_log_path.startswith(str(datadir) + "/")

         self.log.info("Test that the signet data directory with custom -signetchallenge uses network magic as suffix")
-        assert_node_datadir(self.nodes[0], f"signet_{message_start(self.signets[0].challenge)}")
+        assert_node_datadir(self.nodes[0], f"signet")
         assert_node_datadir(self.nodes[4], f"signet_{message_start(self.signets[2].challenge)}")

         self.log.info("Test that the main signet data directory is 'signet'")

$ python feature_signet.py
...
Traceback (most recent call last):
  File "/home/ekzyis/prog/bitcoin/test/functional/test_framework/test_framework.py", line 143, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File "/home/ekzyis/prog/bitcoin/build/test/functional/feature_signet.py", line 120, in run_test
    assert_node_datadir(self.nodes[0], f"signet")
    ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ekzyis/prog/bitcoin/build/test/functional/feature_signet.py", line 112, in assert_node_datadir
    assert_equal(path.basename(datadir), expected_dirname)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ekzyis/prog/bitcoin/test/functional/test_framework/util.py", line 83, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(signet_54d26fbd == signet)

In "Use different datadirs for different signets" 821b15875fec4fc0b211feaaaf5ef10794ff4795

            if signetchallenge.lower() == SIGNET_DEFAULT_CHALLENGE:

nit: match the C++ code lowering signet challenge when comparing signet challenge with signet default challenge

done in a6bde84a3b

In "test: Add signet datadir tests" 3875deab1410672d05325b8e9b561244e190f527

nit: worth adding a test that checks if -signetchallenge=SIGNET_DEFAULT_CHALLENGE.upper() also uses 'signet' datadir

I considered this in #34566 (review), but I didn't really explain my reasoning, so let me do that here:

By "too granular", I meant that it would test behavior that is IMO very easy to verify: the test would verify whether a single line uses ToLower() or not.

Additionally, based on how infrequently CreateBaseChainParams() has changed, I don't think it's likely to change again anytime soon, and I think this will apply to GetSignetDataDir() even more. Because of that, it also doesn't seem like a good candidate for a regression test:

ba8fc7d7889 (TheCharlatan             2023-04-17 22:20:59 +0200 54) std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const ChainType chain)
20e01b1a038 (Pieter Wuille            2014-09-19 19:21:46 +0200 55) {
ba8fc7d7889 (TheCharlatan             2023-04-17 22:20:59 +0200 56)     switch (chain) {
ba8fc7d7889 (TheCharlatan             2023-04-17 22:20:59 +0200 57)     case ChainType::MAIN:
0e2b12b92a2 (Martin Zumsande          2024-11-04 18:24:45 -0500 58)         return std::make_unique<CBaseChainParams>("", 8332);
ba8fc7d7889 (TheCharlatan             2023-04-17 22:20:59 +0200 59)     case ChainType::TESTNET:
0e2b12b92a2 (Martin Zumsande          2024-11-04 18:24:45 -0500 60)         return std::make_unique<CBaseChainParams>("testnet3", 18332);
74a04f9e7ad (Fabian Jahr              2024-03-31 23:50:42 +0200 61)     case ChainType::TESTNET4:
0e2b12b92a2 (Martin Zumsande          2024-11-04 18:24:45 -0500 62)         return std::make_unique<CBaseChainParams>("testnet4", 48332);
ba8fc7d7889 (TheCharlatan             2023-04-17 22:20:59 +0200 63)     case ChainType::SIGNET:
a6bde84a3b8 (ekzyis                   2026-02-11 13:42:45 +0100 64)         return std::make_unique<CBaseChainParams>(GetSignetDataDir(), 38332);
ba8fc7d7889 (TheCharlatan             2023-04-17 22:20:59 +0200 65)     case ChainType::REGTEST:
0e2b12b92a2 (Martin Zumsande          2024-11-04 18:24:45 -0500 66)         return std::make_unique<CBaseChainParams>("regtest", 18443);
e8990f12140 (Karl-Johan Alm           2020-03-05 15:58:30 +0900 67)     }
e23088707be (TheCharlatan             2023-05-09 22:03:50 +0200 68)     assert(false);
84ce18ca933 (Wladimir J. van der Laan 2014-06-19 15:10:04 +0200 69) }

Given that, I don't think it's worth spinning up another node (or restarting one) just to cover this case in the tests.

What do you, or others, think?

I still think it's worth it, but this is still non blocking and up to you

By "too granular", I meant that it would test behavior that is IMO very easy to verify: the test would verify whether a single line uses ToLower() or not.

Test value comes from what happens if the invariant breaks (in this case it would start from a new probably empty datadir), even if it's a single line. It's very easy to verify but also trivial to silently break in a future refactor and this is why I think it's worth it.

Given that, I don't think it's worth spinning up another node (or restarting one) just to cover this case in the tests.

This test already spins 7 nodes, one more isn't a different order of magnitude.

The code under test has been removed, see #34566 (review)

In "Use different datadirs for different signets" 5418bf352a3dcc827739cafedf53a23aa527e334

nit: These changes on test/functional/test_framework/test_node.py introduce a case (chain_dir reads self.extra_args, and start() never updates it) where if restarting a node with a new -signetchallenge it still returns the old chain_dir.

But this is a non issue today and would be only if someone added a test where restart with a new -signetchallenge. I believe this is fine as is and not worth spending time on it. I'm just commenting this so it's on record.

Found this with Opus 5.

I believe this is fine as is and not worth spending time on it. I'm just commenting this so it's on record.

Yes, I agree. Thanks for flagging it! Sounds similar to what I "found" with Claude (https://github.com/bitcoin/bitcoin/pull/34566#discussion_r3668945346), which turned out to test the test framework.

In "doc: Update documentation for signet datadir" 7364f0c7a29aafaeb73cdecbd86c76e9705e01a1

I believe this release note should warn and explain how to upgrade for users that are running with a non default signet challenge. Something like:

Users currently running a custom signet (a non-default -signetchallenge) will have their node select a new, differently-named data directory after upgrading, since previously all signets shared the unsuffixed signet/ directory. To avoid a resync, manually rename/move the existing signet/ directory to the new signet_XXXXXXXX/ path before starting the upgraded node (you can find the suffix in the field magic printed by bitcoin-util -signet -signetchallenge=<hex-script> getchainparams)

This makes me think there should also be a better error message when the user starts on the "wrong" signet:

Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/Users/matthewzipkin/Library/Application Support/Bitcoin/signet/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.

I believe this release note should warn and explain how to upgrade for users that are running with a non default signet challenge.

Good point! Done in 1ae29e4f78. I shortened your suggestion to what I think is the essential information, but please check if you think there's something missing now.

This makes me think there should also be a better error message when the user starts on the "wrong" signet:

Makes sense, but this didn't make it into the current round of addressing feedback. I need to think more about how to improve it. I also don't think it's blocking, so it could be done in a different PR.

I shortened your suggestion to what I think is the essential information, but please check if you think there's something missing now.

Looks good!

In "refactor: Import signet constant and helper from signet.h" 90c55d6528b52ffe91f6caa2fac490a4e05ff1c6

This line and #include <util/strencodings.h> in signet.h means this is also passed to any .cpp that include or transitively include this header which leaks every _hex operator.

This leak can be seen in src/kernel/chainparams.cpp where the #include <util/strencodings.h> was removed although it still uses the operator _hex and still works but it shouldn't rely on the transitive include coming from signet.h.

Suggest moving SIGNET_DEFAULT_CHALLENGE definition into signet.cpp (with an extern const std::vector<uint8_t> SIGNET_DEFAULT_CHALLENGE; declaration in this signet.h), so signet.h doesn't export any hex-literal operators at all.

Nice catch, thank you!! Fixed in 07b10574f1.

In "refactor: Import signet constant and helper from signet.h" 07b10574f1b3950d0d0ecbc15880061fa6e04bd8

I didn't realize before but signet.cpp in this list is for src/test/fuzz/signet.cpp and not for src/signet.cpp. Removing this line would remove the fuzz harness provided by src/test/fuzz/signet.cpp.

This line should be reverted and the commit message updated too.

Wow, that would almost have been my first contribution to fuzzing in Bitcoin Core, haha :sweat_smile:

Thank you, fixed in b3d54514bf!!

07b10574f1b3950d0d0ecbc15880061fa6e04bd8

Do you have to duplicate the long hex string here? Why not import the hex from the header...

diff --git a/src/signet.cpp b/src/signet.cpp
index b55ce20911..fb77b4929a 100644
--- a/src/signet.cpp
+++ b/src/signet.cpp
@@ -27,10 +27,7 @@
 #include <utility>
 #include <vector>
 
-using namespace util::hex_literals;
-
-const std::vector<uint8_t> SIGNET_DEFAULT_CHALLENGE{
-    "512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae"_hex_v_u8};
+const std::vector<uint8_t> SIGNET_DEFAULT_CHALLENGE{ParseHex<uint8_t>(SIGNET_DEFAULT_CHALLENGE_HEX)};
 
 MessageStartChars GetSignetMessageStart(const std::vector<uint8_t>& signet_challenge)
 {

Sorry, was too lazy to mention in advance that I tried this, but it would require adding ../util/strencodings.cpp to the source list of the bitcoinkernel target, and I think we don't want to do that.

However, @rustaceanrob looked at my code, and noticed that I don't even need SIGNET_DEFAULT_CHALLENGE_HEX. I can just parse -signetchallenge as bytes and then compare the bytes, instead of comparing them as (case-insensitive) strings. Then I also happen to not need the test mentioned by @ViniciusCestarii in #34566 (review).

Thanks @rustaceanrob!!

Done in 5418bf352a

In "test: Add signet datadir tests" 40a538ad83c15b1f7857e4f9144a2de9e8096be5

nit: test log is a bit misleading because this is not testing the actual bitcoin-cli -rpcwait.

Right. Looking at what Claude flagged again, it just found a dormant bug in wait_for_cookie_credentials() because I didn't replace self.chain with self.chain_dir. I fixed this in 5418bf352a:

         poll_per_s = 4
         for _ in range(poll_per_s * self.rpc_timeout):
             try:
-                get_auth_cookie(self.datadir_path, self.chain)
+                get_auth_cookie(self.datadir_path, self.chain_dir)
                 self.log.debug("Cookie credentials successfully retrieved")
                 return
             except ValueError:  # cookie file not found and no rpcuser or rpcpassword; bitcoind is still starting
@

(I originally intentionally didn't replace this because it broke no test, but now I changed my mind. I now replaced all occurrences of self.chain in test_node.py.)

This was unrelated to actually calling bitcoin-cli -rpcwait. RPC authentication is already tested well enough in the previous lines afaict.

Since we don't need to test the test framework, I removed this test in e84bcd07c3.

Not blocking, I just noticed this method won't return empty for main net, which bitcoin core actually would. Turns out we do have a few mainnet tests but they set chain = "" 🤷

https://github.com/bitcoin/bitcoin/blob/ab56c78a9929a8e62d377de3dc64d8a81fd85284/test/functional/mining_mainnet.py#L47

Interesting

resolves conversation :smile:

On a more serious note: I think my changes didn't change this (self.chain_dir resolves to self.chain except if a custom challenge is passed to -signetchallenge), so not sure if there's something to do here

5418bf352a3dcc827739cafedf53a23aa527e334

Nit:

ReadSigNetArgs() isn't called from bitcoin-cli so you could this slightly confusing behavior:

--> bccli -signet -signetchallenge=z  getblockcount
error: Authorization failed: Failed to read cookie file and no rpcpassword was specified. Configuration file: (/Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf)
Did you forget -signetchallenge for custom signets?

Mhh, this looks like a good follow-up!

Before my changes, -signetchallenge didn't matter to bitcoin-cli, so my changes exposed this behavior. However, I think we want separate Concept ACKs for fixing this, for example by calling SelectParams() in the RPC client instead of only SelectBaseParams() (might be too heavy-handed).

I don't think this really makes sense -- signet.cpp is mostly validation logic, so shouldn't be added to common.

I think just adding a dedicated header would be better, perhaps kernel/signet_default.h:

#ifndef BITCOIN_KERNEL_SIGNET_DEFAULT_H
#define BITCOIN_KERNEL_SIGNET_DEFAULT_H

#include <hash.h>
#include <kernel/messagestartchars.h>
#include <uint256.h>
#include <util/strencodings.h>

#include <algorithm>
#include <cstdint>
#include <vector>

namespace kernel {
using namespace util::hex_literals;
inline const std::vector<uint8_t> SIGNET_DEFAULT_CHALLENGE{
    "512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae"_hex_v_u8
};

inline MessageStartChars GetSignetMessageStart(const std::vector<uint8_t>& signet_challenge)
{
    HashWriter h{};
    h << signet_challenge;
    const uint256 hash = h.GetHash();
    MessageStartChars msg_start;
    std::copy_n(hash.begin(), 4, msg_start.begin());
    return msg_start;
}
} // kernel namespace

#endif // BITCOIN_KERNEL_SIGNET_DEFAULT_H

Ohh, this requires no changes to any CMakeLists.txt, nice!

Done in 75501ddc54. I named the file kernel/signet.h instead of kernel/signet_default.h because GetSignetMessageStart is IMO about more than just the default signet and to be consistent with other headers (chainparams.h and kernel/chainparams.h).

Might be clearer if gArgs were passed as an argument to CreateBaseChainParams and GetSignetDataDir rather than be a global with ordering dependencies?

Makes sense! Done in 67a057eb91 and removed the comment. I think the order is now clear based on what CreateBaseChainParams accepts. But it's not enforced because you could still pass gArgs before calling gArgs.SelectConfigNetwork(chainType). Lmk if you think the comment still makes sense.

I think this should be auto challenge_hex = gArgs.GetArg("-signetchallenge"); (no default, returns an optional), so that -signetchallenge= is interpreted as an empty challenge and can be treated as distinct from the default challenge. I believe this matches the behaviour in chainparams.cpp.

I see that on master, we support -signetchallenge= as its own distinct network. My code here would interpret it as the default challenge, so there's def a mismatch here, thank you! Fixed in 67a057eb91 and added a test in 1fbfaa11a4.

However, is it intentional to allow empty -signetchallenge=? Does a signet without any challenge make sense? I thought there must be a challenge for the miner, or at least a trivial challenge like OP_TRUE.