Expands the benchmark framework with the existing -testdatadir arg,
enabling the ability to change the benchmark data directory.
This is useful for running benchmarks on different storage devices, and
not just under the OS /tmp/ directory.
A good use case is #28574, where we are benchmarking the wallet migration process on an HDD.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31000.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | tdb3, hodlinator, maflcko, pablomartin4btc, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30825058976</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
Just as a note, it should already be possible to pick the device via an env var, see https://en.cppreference.com/w/cpp/filesystem/temp_directory_path#Notes.
Efficient code
Just as a note, it should already be possible to pick the device via an env var, see https://en.cppreference.com/w/cpp/filesystem/temp_directory_path#Notes.
Hmm, thats a good observation.
Even when that may be possible now, I'm not sure we should start recommending the mixture of env vars and binary args at the user interface level. It seems more natural to plug the already existing -testdatadir arg to the benchmark framework. Still, if we move towards the env var direction, the next step should be revert #26564?.
the next step should be revert #26564?.
IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
It seems more natural to plug the already existing -testdatadir arg to the benchmark framework.
I agree. I've also suggested this in #30737 (review)
the next step should be revert #26564?.
IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
Practically speaking, we're ultimately talking about pretty much the same feature. It's about customizing the test root directory path so it can be inspected or used in another context. The thing is, 26564 introduced several different features in the same commit: the test no cleanup, the directory name specialization using the test name, and the -testdatadir arg. And what I meant is that this last point can be achieved by setting the environment variable you mentioned. Yet, it seems we agree on not doing it, but it's curious that this can still be used nowadays.
60 | @@ -60,6 +61,17 @@ static uint8_t parsePriorityLevel(const std::string& str) { 61 | return levels; 62 | } 63 | 64 | +// Parses test setup related arguments 65 | +static std::vector<std::string> parseTestSetupArgs(const ArgsManager& argsman) {
Braces on new lines for classes, functions, methods.
Can see you are following style of function above, but function below follows the dev-notes.
Don't feel as strongly about capitalization.
static std::vector<std::string> ParseTestSetupArgs(const ArgsManager& argsman)
{
If you want to make parsePriorityLevel conform to the dev-notes as well, I support it.
Done. Added the jump line. I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.
I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.
(SetupBenchArgs is capitalized, main must be non-capitalized, so that only leaves parseAsymptote to change beyond the function under discussion).
60 | @@ -60,6 +61,17 @@ static uint8_t parsePriorityLevel(const std::string& str) { 61 | return levels; 62 | } 63 | 64 | +// Parses test setup related arguments 65 | +static std::vector<std::string> parseTestSetupArgs(const ArgsManager& argsman) { 66 | + std::vector<std::string> args; 67 | + 68 | + // Test data directory
nit: Redundant comment.
Have reworked this function in a more general manner just so we can add new args in the future with just one line of parsing code.
A bit heavy for one arg as it is right now, but I like that it avoids repeating the name. :+1:
36 | @@ -37,6 +37,7 @@ static void SetupBenchArgs(ArgsManager& argsman) 37 | argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration with no output", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); 38 | argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of one or multiple priority level(s) (%s), default: '%s'", 39 | benchmark::ListPriorities(), DEFAULT_PRIORITY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); 40 | + argsman.AddArg("-testdatadir", "Run benchmarks on a custom directory (default: </tmp/>)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
Why not make SetupUnitTestArgs available and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.
(Could maybe rename SetupUnitTestArgs -> SetupTestArgs or static BasicTestingSetup::RegisterArgs to make it less unit test-specific and communicate how broadly it is used).
Why not make
SetupUnitTestArgsavailable and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.(Could maybe rename
SetupUnitTestArgs->SetupTestArgsorstatic BasicTestingSetup::RegisterArgsto make it less unit test-specific and communicate how broadly it is used).
I think we should be careful and explicit about the features we add to other binaries. One of them could end up clashing with another functionality or be removed from a binary that was using it without us noticing it.
Agree a SetupTestArgs may not communicate clearly enough that the function is used in a broader context. How about calling it SetupCommonTestArgs and adding a comment about it being used in benchmarks as well? That, together with no longer marking the function static (private to the compilation unit), might mitigate your concerns?
Agree a
SetupTestArgsmay not communicate clearly enough that the function is used in a broader context. How about calling itSetupCommonTestArgsand adding a comment about it being used in benchmarks as well? That, together with no longer marking the functionstatic(private to the compilation unit), might mitigate your concerns?
I'm not so in favor of mentioning upper level binaries or frameworks in lower level files but no problem in applying the suggestion.
Hmm well, I'm still a bit reluctant to it. It would add test/util/setup_common.h dependency to the benchmark binary entry point just for a single line of code.
Adding a dependency on setup_common.h is a case of making implicit dependencies explicit in my view. What the current version signifies is a different argument description (starting with "Run benchmarks " like the others) and a default directory value that can't be as clear as the original. We shouldn't be duplicating code snippets to avoid #includes. Even if it minimizes the size of this specific PR, going forward one has to make sure to keep both the original and duplicated place up to date which I argue is a worse state of tech debt.
I'm happy to hear stronger counter-arguments if you feel it's worth your time, but I can understand if you want to drop this subject too.
Summarizing my intent:
diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp
index 3d649a8436..3ae3f7dd4c 100644
--- a/src/bench/bench_bitcoin.cpp
+++ b/src/bench/bench_bitcoin.cpp
@@ -8,6 +8,7 @@
#include <tinyformat.h>
#include <util/fs.h>
#include <util/string.h>
+#include <test/util/setup_common.h>
#include <chrono>
#include <cstdint>
@@ -37,7 +38,7 @@ static void SetupBenchArgs(ArgsManager& argsman)
argsman.AddArg("-sanity-check", "Run benchmarks for only one iteration with no output", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-priority-level=<l1,l2,l3>", strprintf("Run benchmarks of one or multiple priority level(s) (%s), default: '%s'",
benchmark::ListPriorities(), DEFAULT_PRIORITY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
- argsman.AddArg("-testdatadir", "Run benchmarks on a custom directory (default: </tmp/>)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+ SetupCommonTestArgs(argsman);
}
// parses a comma separated list like "10,20,30,50"
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index fa89ceb332..2471290fe9 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -87,8 +87,8 @@ struct NetworkSetup
};
static NetworkSetup g_networksetup_instance;
-/** Register test-only arguments */
-static void SetupUnitTestArgs(ArgsManager& argsman)
+/** Register common test and bench arguments */
+void SetupCommonTestArgs(ArgsManager& argsman)
{
argsman.AddArg("-testdatadir", strprintf("Custom data directory (default: %s<random_string>)", fs::PathToString(fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / "")),
ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
@@ -126,7 +126,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
gArgs.ClearPathCache();
{
SetupServerArgs(*m_node.args);
- SetupUnitTestArgs(*m_node.args);
+ SetupCommonTestArgs(*m_node.args);
std::string error;
if (!m_node.args->ParseParameters(arguments.size(), arguments.data(), error)) {
m_node.args->ClearArgs();
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index f5a3c0dcee..4d8b9370ca 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -45,6 +45,8 @@ extern const std::function<std::string()> G_TEST_GET_FULL_NAME;
static constexpr CAmount CENT{1000000};
+void SetupCommonTestArgs(ArgsManager& argsman);
+
struct TestOpts {
std::vector<const char*> extra_args{};
bool coins_db_in_memory{true};
I do think that referencing upper-level binaries in lower-level framework code isn't ideal, and duplicating a single line for an argument that rarely changes behavior is harmless. Especially when the small deduplication comes with other unnecessary stuff and isn't truly a duplication as the help text is customized for benchmark binary users. But, that said, we're already breaking the first point elsewhere in setup_common.h, so np. Will re-touch a bit your suggestion and push it.
32 | + * Retrieves the available test setup command line arguments that may be used 33 | + * in the benchmark. They will be used only if the benchmark utilizes a 34 | + * 'BasicTestingSetup' or any child of it. 35 | + */ 36 | +static std::function<std::vector<const char*>()> g_bench_command_line_args{}; 37 | +
(Please either remove empty line after local global here or add an empty line after static std::string g_running_benchmark_name).
done.
Concept ACK 1ba225c6ce689defb7063dd68f62e1e90fcdda5e
Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in #30737.
Thanks for the review @hodlinator. Updated per feedback.
Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in #30737.
Cool. It wasn't on my radar. Adding it now.
git range-diff master 1ba225c ded1a6c
Still feel like it would be good to call a common function adding -testdatadir to an ArgsManager instead of duplicating the functionality between test/bench (and having a different description-string).
tACK ded1a6cc498ffde0695eb3ee6828b7c513ccc277
<details> <summary>Tested on Ubuntu 22.04 locally and using an external USB.</summary>
./build/src/bench/bench_bitcoin -filter=Xor -testdatadir=/tmp/btc
Warning, results might be unstable:
* DEBUG defined
* CPU frequency scaling enabled: CPU 0 between 400.0 and 4,700.0 MHz
* CPU governor is 'powersave' but should be 'performance'
* Turbo is enabled, CPU frequency will fluctuate
Recommendations
* Make sure you compile for Release
* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 9.24 | 108,266,754.90 | 1.5% | 0.01 | `Xor`
./build/src/bench/bench_bitcoin -filter=Xor -testdatadir=/media/pablo/USB\ DISK/tmp/btc
Warning, results might be unstable:
* DEBUG defined
* CPU frequency scaling enabled: CPU 0 between 400.0 and 4,700.0 MHz
* CPU governor is 'powersave' but should be 'performance'
* Turbo is enabled, CPU frequency will fluctuate
Recommendations
* Make sure you compile for Release
* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
| ns/byte | byte/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 11.52 | 86,792,532.79 | 28.6% | 0.01 | :wavy_dash: `Xor` (Unstable with ~45.2 iters. Increase `minEpochIterations` to e.g. 452)
</details>
I think it's a nice feature, even this can be achieved by changing the env var (e.g. TMPDIR=/my_temp ./build/src/bench/bench_bitcoin), using -testdatadir feels more consistent with other binaries use cases.
Approach ACK Useful improvement.
Did a quick sanity check for now (ramdisk at /mnt/tmp/). Planning to circle back.
$ build/src/bench/bench_bitcoin -testdatadir=/mnt/tmp/
$ ls /mnt/tmp/test_common\ bitcoin
AssembleBlock DuplicateInputs MempoolCheck WalletBalanceDirty WalletIsMineDescriptors
BlockAssemblerAddPackageTxns LoadExternalBlockFile MempoolEviction WalletBalanceMine WalletIsMineMigratedDescriptors
BlockFilterIndexSync LogWithDebug ReadBlockFromDiskTest WalletBalanceWatch WalletLoadingDescriptors
BlockToJsonVerbose LogWithoutDebug ReadRawBlockFromDiskTest WalletCreateEncrypted
BlockToJsonVerboseWrite LogWithoutThreadNames RpcMempool WalletCreatePlain
CheckBlockIndex LogWithoutWriteToFile WalletAvailableCoins WalletCreateTxUseOnlyPresetInputs
ComplexMemPool LogWithThreadNames WalletBalanceClean WalletCreateTxUsePresetInputsAndCoinSelection
27 | @@ -27,6 +28,7 @@ static const std::string DEFAULT_PRIORITY{"all"}; 28 | static void SetupBenchArgs(ArgsManager& argsman) 29 | { 30 | SetupHelpOptions(argsman); 31 | + SetupCommonTestArgs(argsman);
I quickly glanced over this PR only, do I understand it correctly that we're treating benchmarks as tests here (I'm not sure I agree with that concept) and adding parameters to the benchmark setup that only apply to a smaller subset of the benchmarks?
Could we configure them via env variables instead and call the benchmark with e.g. TEST_DATA_DIR=a/b/c bench instead?
As an example, if a few of our tests require a for example a timestamp for whatever reason, we wouldn't add it to the testing framework as an additional parameter, right?
So if my understanding is correct, I'm leaning towards a concept NACK - please let me know if I misunderstood it.
I quickly glanced over this PR only, do I understand it correctly that we're treating benchmarks as tests here (I'm not sure I agree with that concept)
I'm not sure this is the best place for the conversation, but generally speaking, benchmarks are a form of testing. The difference is that they evaluate performance against previous runs rather than behavior.
adding parameters to the benchmark setup that only apply to a smaller subset of the benchmarks?
This applies to every benchmark requiring a node context. We currently have 33 of them.
Could we configure them via env variables instead and call the benchmark with e.g. TEST_DATA_DIR=a/b/c bench instead?
Please check the conversation above #31000 (comment).
As an example, if a few of our tests require a for example a timestamp for whatever reason, we wouldn't add it to the testing framework as an additional parameter, right?
Unlike the hardware device type the benchmark runs on, which can't be standardized for all users, software level variables like timestamps can be set in a general manner if needed. I don't think there will be many args like this one.
So if my understanding is correct, I'm leaning towards a concept NACK - please let me know if I misunderstood it.
I think you understood it. Just have a different opinion.
I did see that conversation, but I don't think it's the benchmark's responsibility to reveal the internals of a certain group of tests.
ACK 15cfeebb47587af6ce7be72fd52c57a0483d86d2
Thanks @furszy for making the dependency explicit! I don't fully understand the upper/lower level argument as bench_bitcoin has a link dependency on test_util already.
Using ENV-vars instead as suggested in other comment seems too sneaky for me, and contradicts my goal of having things explicit. (IMO one original sin of C was main not taking environment args as a parameter). 31 tests are using BasicTestingSetup so I think it's common enough to be supported at a benchmark executor level.
â‚¿ build/src/bench/bench_bitcoin -testdatadir=foo/
...
Test directory (will not be deleted): "/home/hodlinator/bitcoin/foo/test_common bitcoin/WalletIsMineDescriptors/datadir"
...
â‚¿ ls foo/test_common\ bitcoin/
AssembleBlock ComplexMemPool LogWithoutWriteToFile RpcMempool WalletCreateEncrypted WalletLoadingDescriptors
BlockAssemblerAddPackageTxns DuplicateInputs LogWithThreadNames WalletAvailableCoins WalletCreatePlain
BlockFilterIndexSync LoadExternalBlockFile MempoolCheck WalletBalanceClean WalletCreateTxUseOnlyPresetInputs
BlockToJsonVerbose LogWithDebug MempoolEviction WalletBalanceDirty WalletCreateTxUsePresetInputsAndCoinSelection
BlockToJsonVerboseWrite LogWithoutDebug ReadBlockFromDiskTest WalletBalanceMine WalletIsMineDescriptors
CheckBlockIndex LogWithoutThreadNames ReadRawBlockFromDiskTest WalletBalanceWatch WalletIsMineMigratedDescriptors
lgtm ACK 15cfeebb47587af6ce7be72fd52c57a0483d86d2
re-ACK 15cfeebb47587af6ce7be72fd52c57a0483d86d2
Changes since my last review: removing the -testdatadir arg from bench-bitcoin and reusing it from the utils setup_common.
In 221410bc14616eaf7017e5097cd6848fcb4f8e1b "bench: specialize working directory name" , the commit message says
After this commit, benchmarks are contained within its own directory: /<OS_tmp_dir>/test_common bitcoin/<benchmark_name>/<random_uint256>/
However I am not observing this behavior. It omits the benchmark name.
The benchmark name is included when -testdatadir is used.
In 221410b "bench: specialize working directory name" , the commit message says
After this commit, benchmarks are contained within its own directory: /<OS_tmp_dir>/test_common bitcoin/<benchmark_name>/<random_uint256>/
However I am not observing this behavior. It omits the benchmark name.
The benchmark name is included when
-testdatadiris used.
Yeah, that's because the data directory is erased after execution when -testdatadir isn't used. And the same applies to unit tests. That's why I didn't change it, though I could for consistency, making it easier to find the test data directory in case of failure - which is when the directory is not erased.
Updated per feedback. Thanks achow.
re-ACK 5757fdf0dc74ec9d6bbefba937d6e23b09652605
A small improvement since my last review, adding (also) benchmark name/ test name to the test common dir when -testdatadir is not specified.
Needs rebase
142 | if (!m_node.args->IsArgSet("-testdatadir")) {
143 | - // By default, the data directory has a random name
144 | + // By default, the data directory has a random name on each test run
145 | const auto rand_str{g_rng_temp_path.rand256().ToString()};
146 | - m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / rand_str;
147 | + m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand_str;
(Since with the latest non-rebase push you are now touching 2/4 lines mentioning TEST_DIR_PATH_ELEMENT, iff you were to re-touch again, it would be nice if you also renamed it to TESTS_DIR_NAME as per @ryanofsky's suggestion).
re-ACK 80d35bc20797e59571e82fe6919705b33dcc40cd
test_path -> test_name (improved naming).test_name be included as part of randomized test directory when not setting -testdatadir. Including it is helpful for locating test data of a specific test when multiple tests fail. :+1:Verified correct unit test directory behavior using build/src/test/test_bitcoin -- -testdatadir=/tmp/foo/, then not passing the -testdatadir and simultaneously running lsof |grep bitcoin |grep /tmp.
Not sure about the CI failure. IIRC it happened after this push: https://github.com/bitcoin/bitcoin/compare/15cfeebb47587af6ce7be72fd52c57a0483d86d2..5757fdf0dc74ec9d6bbefba937d6e23b09652605
The logs don't say anything except: validation_chainstatemanager_tests ...Exit code 0xc0000409 2024-10-24T23:23:18.2978457Z ***Exception: 9.33 sec and the debug log is truncated (presumably when the exception happens).
It would be good if ctest/Windows actually printed the exception and error message instead of silently hiding it.
cc @hebasto
I checked for a MAX_PATH violation, but while plausible, I couldn't find evidence:
>>> len(r'C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\validation_chainstatemanager_tests\chainstatemanager_rebalance_caches\f487e4068280f4a48936848359cd7101a5000f55b4c7d05c531cd317544df024\regtest\chainstate')
210
Looks like that fixed it. However, it would be good to understand why it fixed it. Looking at the diff that triggered it, I fail to see how (https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680)
Looks like that fixed it. However, it would be good to understand why it fixed it. Looking at the diff that triggered it, I fail to see how (#31000 (comment))
Running on another branch with -testdatadir=foo I get files like:
foo/test_common bitcoin/validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch/datadir/regtest/chainstate_snapshot_INVALID/MANIFEST-000004
Combined with C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\validation_chainstatemanager_tests\chainstatemanager_rebalance_caches\f487e4068280f4a48936848359cd7101a5000f55b4c7d05c531cd317544df024 from above into something like C:\Users\RUNNER~1\AppData\Local\Temp/test_common bitcoin/validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch/f487e4068280f4a48936848359cd7101a5000f55b4c7d05c531cd317544df024/datadir/regtest/chainstate_snapshot_INVALID/MANIFEST-000004 we get 268 chars, while MAX_PATH is 260.
Edit: Does not sufficiently explain the CI failure as CI does not set -testdatadir and so would not end up with the random 256 bit hex values in the paths, as pointed out by maflcko in the following comment.
Running on another branch with
-testdatadir=foo
The Windows CI doesn't set the flag and the push/diff I linked to didn't change the non -testdatadir code path, but the previous commit passed and the next one failed. Just asking to understand what is going on with the CI.
140 | @@ -141,7 +141,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) 141 | const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""}; 142 | if (!m_node.args->IsArgSet("-testdatadir")) { 143 | // By default, the data directory has a random name on each test run 144 | - const auto rand_str{g_rng_temp_path.rand256().ToString()}; 145 | + const auto rand_str{util::ToString(g_rng_temp_path.rand32())};
nit: for awareness, may want to mention in OP that this changes directory names from 256-bit randoms to 32-bit randoms. I'm not seeing an issue with it (virtually non-existent likelihood of collision).
ACK ebdace5aaf74626da83eeee1f966acaad3ed0c35
Light code review and tested.
$ build/src/bench/bench_bitcoin -testdatadir=/mnt/31000/
...
$ ls /mnt/31000/test_common\ bitcoin/
AssembleBlock LoadExternalBlockFile ReadBlockFromDiskTest WalletCreateEncrypted
BlockAssemblerAddPackageTxns LogWithDebug ReadRawBlockFromDiskTest WalletCreatePlain
BlockFilterIndexSync LogWithThreadNames RpcMempool WalletCreateTxUseOnlyPresetInputs
BlockToJsonVerbose LogWithoutDebug WalletAvailableCoins WalletCreateTxUsePresetInputsAndCoinSelection
BlockToJsonVerboseWrite LogWithoutThreadNames WalletBalanceClean WalletIsMineDescriptors
CheckBlockIndex LogWithoutWriteToFile WalletBalanceDirty WalletIsMineMigratedDescriptors
ComplexMemPool MempoolCheck WalletBalanceMine WalletLoadingDescriptors
DuplicateInputs MempoolEviction WalletBalanceWatch
re: #31000 (comment)
It would be good if ctest/Windows actually printed the exception and error message instead of silently hiding it.
I'm not aware of the specific case discussed above, but exceptions in tests are generally printed as follows (with a patched code):
> ctest --test-dir build-static -j 8 -C Release -R amount --output-on-failure
Internal ctest changing into directory: C:/Users/hebasto/bitcoin/build-static
Test project C:/Users/hebasto/bitcoin/build-static
Start 10: amount_tests
1/1 Test [#10](/bitcoin-bitcoin/10/): amount_tests .....................***Failed 0.04 sec
Running 4 test cases...
Entering test module "Bitcoin Core Test Suite"
C:\Users\hebasto\bitcoin\src\test\addrman_tests.cpp(63): Test suite "addrman_tests" is skipped because disabled
C:\Users\hebasto\bitcoin\src\test\allocator_tests.cpp(16): Test suite "allocator_tests" is skipped because disabled
C:\Users\hebasto\bitcoin\src\test\amount_tests.cpp(12): Entering test suite "amount_tests"
C:\Users\hebasto\bitcoin\src\test\amount_tests.cpp(14): Entering test case "MoneyRangeTest"
C:\Users\hebasto\bitcoin\src\test\amount_tests.cpp(14): Leaving test case "MoneyRangeTest"; testing time: 14us
C:\Users\hebasto\bitcoin\src\test\amount_tests.cpp(23): Entering test case "GetFeeTest"
unknown location(0): fatal error: in "amount_tests/GetFeeTest": class std::runtime_error: A runtime error occurred
C:\Users\hebasto\bitcoin\src\test\amount_tests.cpp(59): last checkpoint
...
It would be good if ctest/Windows actually printed the exception and error message instead of silently hiding it.
I'm not aware of the specific case discussed above, but exceptions in tests are generally printed as follows (with a patched code):
Maybe the log is truncated in CI and only the head is printed, as opposed to the tail, which would be more useful?
Sorry for the delay. @maflcko, the issue seems to be that the exception occurs in the test base class constructor. Which is not part of the actual test code and runs prior to it. So it might not be captured by the framework, throwing the "unknown location error". Just dropped the fix and pushed a commit to verify it. Let's see what the CI prints now.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/32605835023</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
@maflcko, have tried a good number of different things to catch the error but didn't have much luck in the CI. ctest seems to be swallowing the error there. It occurs on a different place, not during the initial root directory creation. It probably occurs in one of the blk**.dat or the ldb files, they are also created inside the TestChain100Setup constructor.
Still, we could at least move forward with something like
https://github.com/bitcoin/bitcoin/pull/31000/commits/fd5b9fc36879ead0efb12748cf8c4b9e60d7c59c, which prints a better error message when the base dir exceeds the max path limit. And could also even limit the path size further, reducing it by the longest path we have: MAX_PATH - len(/indexes/blockfilter/basic/db/MANIFEST-000002).
This seems orthogonal to this PR, probably should move it to another PR, but happy to hear your thoughts too.
Yeah, don't worry about the ctest bug. It is unrelated and should be fixed somewhere else. I was mostly wondering why a force push that does not change behavior (https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680) suddenly makes the CI trip. Why would CI pass before and not after, when -testdatadir isn't set. Maybe someone can run the two commits locally on Windows?
Yeah, don't worry about the ctest bug. It is unrelated and should be fixed somewhere else. I was mostly wondering why a force push that does not change behavior (#31000 (comment)) suddenly makes the CI trip. Why would CI pass before and not after, when
-testdatadirisn't set. Maybe someone can run the two commits locally on Windows?
Ok, that force-push actually contained a behavior change. It changed the default test datadir path. Moved from
/<OS_tmp_dir>/test_common bitcoin/<random_uint256>/ to /<OS_tmp_dir>/test_common bitcoin/<test_name>/< random_uint256>/. This makes it easier to find any unit/benchmark/fuzzing test run when a failure occurs. Instead of going through all the random uint256 directories to see all the tests executions, we can just open the specific test name folder.
Thus why I changed the random uint256 to be a uint32 in the fix commit. It doesn't need to be a long random number anymore.
Updated. Ready to go.
Ok, that force-push actually contained a behavior change.
Ugh, I missed that, I didn't see the ! negation. Generally writing a simple if with an else branch as if (!a) ... is confusing, because each branch will have a (double) negation. It would be better to just write if (a) and have a single negation in the else branch. But you didn't change that code, so it can be done in a follow-up.
142 | if (!m_node.args->IsArgSet("-testdatadir")) {
143 | - // By default, the data directory has a random name
144 | - const auto rand_str{g_rng_temp_path.rand256().ToString()};
145 | - m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / rand_str;
146 | + // By default, the data directory has a random name on each test run
147 | + const auto rand_str{util::ToString(g_rng_temp_path.rand32())};
nit: My preference would be to use the current time (maybe in nanosecond precision) and make the path time/test_name. This way, it is easy to find the last test execution easily, if there is more than one.
With the current approach, one would have to look up which rand32 corresponds to the last execution.
sure. Done as suggested.
re-ACK 4bbcc704464a470f54f372f93b2649d67b7a60c1
Only change is the default behavior change, which I somehow missed.
143 | if (!m_node.args->IsArgSet("-testdatadir")) {
144 | - // By default, the data directory has a random name
145 | - const auto rand_str{g_rng_temp_path.rand256().ToString()};
146 | - m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / rand_str;
147 | + // By default, the data directory has a random name on each test run
148 | + const auto rand_str{util::ToString(GetTime<std::chrono::nanoseconds>().count())};
non-blocking nit:
Since time is being used now, the name isn't quite as random as before. Could update the comment and rand_str name.
sure, pushed. Thanks
re ACK 5adb4a44776de3f9295cd6bf965f8beba0e4e368
Change since last push is a simple removal of a semi-accurate comment.
142 | if (!m_node.args->IsArgSet("-testdatadir")) {
143 | - // By default, the data directory has a random name
144 | - const auto rand_str{g_rng_temp_path.rand256().ToString()};
145 | - m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / rand_str;
146 | + const auto rand_str{util::ToString(GetTime<std::chrono::nanoseconds>().count())};
147 | + m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand_str;
I don't think it is right to use mockable time here. Also, GetTime is deprecated, so what about TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())?
Also, I think it would be better to switch test_name / rand_str to rand_str / test_name, so that all unit tests from the same time are bundled together. This would also be identical to the functional test runner.
I don't think it is right to use mockable time here. Also, GetTime is deprecated, so what about TicksSinceEpochstd::chrono::nanoseconds(SystemClock::now())?
Yeah sure. This is usually called before mocking the time, but agree to change it.
Also, I think it would be better to switch test_name / rand_str to rand_str / test_name, so that all unit tests from the same time are bundled together. This would also be identical to the functional test runner.
That won’t work as you expect. Think of each unit test as a separate child class inheriting from the testing setup class and implementing a run() method. Each one constructs a new instance of the setup class, thereby creating a new rand_str.
I think we might be able to switch to time / test_name, creating a global testing class with a different Boost framework hook, but I’d prefer to leave that for a follow-up since it’s somewhat unrelated to this PR and will require additional investigation.
re-ACK 6c7b3bf0ce7a6238457dd44ee1d5d744b3663a52
code review and light test re ACK 6c7b3bf0ce7a6238457dd44ee1d5d744b3663a52
build/src/bench/bench_bitcoin:
...
/tmp/test_common bitcoin/BlockAssemblerAddPackageTxns/1731341243673972130/regtest/blocks
Update uses TicksSinceEpoch with SystemClock::now() instead of deprecated GetTime().
142 | if (!m_node.args->IsArgSet("-testdatadir")) {
143 | - // By default, the data directory has a random name
144 | - const auto rand_str{g_rng_temp_path.rand256().ToString()};
145 | - m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / rand_str;
146 | + const auto now{TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())};
147 | + m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / util::ToString(now);
nit: Could potentially move the ToString call up with the rest of the calculation for that value.
const auto now{util::ToString(TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now()))};
m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / now;
I prefer the current approach to not over-extend the first line. Even with the ToString call, the second line is simpler to read.
137 | @@ -139,10 +138,10 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts) 138 | // data directories use a random name that doesn't overlap with other tests. 139 | SeedRandomForTest(SeedRand::FIXED_SEED); 140 | 141 | + const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""}; 142 | if (!m_node.args->IsArgSet("-testdatadir")) { 143 | - // By default, the data directory has a random name 144 | - const auto rand_str{g_rng_temp_path.rand256().ToString()};
As g_rng_temp_path is no longer used, it should be removed completely from line 78-79.
Yeah good, done as suggested. Thanks.
Since G_TEST_GET_FULL_NAME is not initialized in the benchmark framework,
benchmarks using the unit test setup run in the same directory without
any clear distinction between them.
This poses an extra complication for locating any specific benchmark
directory during a failure.
In master, unit tests and benchmarks run in the following path:
/<OS_tmp_dir>/test_common bitcoin/<random_uint256>/
After this commit, unit tests and benchmarks are contained within its
own directory:
/<OS_tmp_dir>/test_common bitcoin/<test_name>/<time_in_nanoseconds>/
This makes it easier to find any benchmark run when a failure occurs.
Expands the benchmark framework with the existing '-testdatadir' arg,
enabling the ability to change the benchmark data directory.
This is useful for running benchmarks on different storage devices, and
not just under the OS /tmp/ directory.
Updated per feedback. Thanks. Removed unused global g_rng_temp_path from the setup common file.
re ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
git range-diff master 80d35bc fa66e08
Sample of recent now-value is 1731341566994144855.
According to https://www.epochconverter.com/ we won't get another digit until year 2286, so path lengths seem quite stable.
Switching to hex representation does give us constant lengths from now until unsigned 64-bit max, but only shaves off 3 chars, which is not that big of a win.
The paths were made more deterministic in 97e16f57042cab07e5e73f6bed19feec2006e4f7, but I suspect it's not that much of a priority.
I tried to follow through on the determinism intent in my #30737 but the approach in your PR should also fix #30696 (might be worth mentioning that you're fixing that issue as a bonus in the PR summary! :) ).
TEST_DIR_PATH_ELEMENTCould potentially still rename TEST_DIR_PATH_ELEMENT -> TESTS_DIR_NAME as previously mentioned.
Ran build/src/test/test_bitcoin and lsof |grep "/tmp.*bitcoin" in parallel.
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
Only change is removing unused g_rng_temp_path
re-ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048
Since my last review: removal of g_rng_temp_path, renaming and moving definition of SetupCommonTestArgs to src/test/util/setup_common.h and using the timestamp for the test dir path instead of a random id.
ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048