Skip to content

test: Revert to random path element #31317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fanquake merged 2 commits into from
Nov 20, 2024
Merged

test: Revert to random path element #31317

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa80b08
test: Revert to random path element
Nov 18, 2024
faaaf59
test: Make g_rng_temp_path rand, not dependent on SeedRandomForTest
Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/test/util/setup_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@ using node::VerifyLoadedChainstate;
const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;

constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */

@hodlinator hodlinator Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested cherrypicking 8e5f8a4 from #30737 on top of this PR to forward RANDOM_CTX_SEED on inside test/fuzz/test_runner.py.

Ran this ~10 times:

RANDOM_CTX_SEED=21 build_fuzz/test/fuzz/test_runner.py ../qa-assets/

Many of the failures come down to:

Done 2 runs in 0 second(s)
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: cannot remove all: No such file or directory [/tmp/test_common bitcoin/02ccd0019e3e40fb6001] [/tmp/test_common bitcoin/02ccd0019e3e40fb6001/regtest]
==41419== ERROR: libFuzzer: deadly signal

This proves to me that the latter part of this comment is not true. I would therefore suggest one of:

  1. Use m_rng directly instead of reintroducing g_rng_temp_path.
  2. Make g_rng_temp_path truly non-deterministic (maybe through reverting part of 97e16f5, moving the seeding, haven't tested).
  3. Remove the latter part of the comment. (Edit: This defeats the point of introducing g_rng_temp_path, so the other two alternatives would be preferable).
Suggested change
/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
/** Random context to get unique temp data dirs. */

@maflcko maflcko Nov 19, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this issue is re-introduced.

It can also be tested on top of fa80b08 via something like: RANDOM_CTX_SEED=21 FUZZ=utxo_total_supply ./bld-cmake/src/test/fuzz/fuzz -runs=10 -jobs=2 -workers=2

I didn't want to fix it, as it was pre-existing and no one complained (at least OSS-Fuzz did not), whereas OSS-Fuzz did complain about the fixed issue.

However, as this is a trivial fix, I've added another commit to do so.

Let me know if this is acceptable. If not, my recommendation would be to move it to a follow-up to not delay this fix further.

@hodlinator hodlinator Nov 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having g_rng_temp_path_init be called as soon as this compilation unit is included in a binary isn't ideal but still acceptable (BasicTestingSetup may not be used in a given run).

Good call on removing the defunct comment before SeedRandomForTest() as well.

@maflcko maflcko Nov 20, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having g_rng_temp_path_init be called as soon as this compilation unit is included in a binary isn't ideal but still acceptable (BasicTestingSetup may not be used in a given run).

Yeah, I guess it should be fine to move the init inside the BasicTestingSetup constructor, as long as it is before the SeedRandomForTest. I am happy to review a follow-up doing that, but I'll leave this as-is for now, due to the three acks.

@hodlinator hodlinator Nov 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the robustness of the current solution. Never know where SeedRandomForTest might creep in inside unit/bench/fuzz. Please resolve this.

static FastRandomContext g_rng_temp_path;
static const bool g_rng_temp_path_init{[] {
// Must be initialized before any SeedRandomForTest
(void)g_rng_temp_path.rand64();
return true;
}()};

struct NetworkSetup
{
Expand Down Expand Up @@ -132,14 +139,16 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
}
}

// Use randomly chosen seed for deterministic PRNG, so that (by default) test
// data directories use a random name that doesn't overlap with other tests.
SeedRandomForTest(SeedRand::FIXED_SEED);

const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
if (!m_node.args->IsArgSet("-testdatadir")) {
const auto now{TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())};
m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / util::ToString(now);
// To avoid colliding with a leftover prior datadir, and to allow
// tests, such as the fuzz tests to run in several processes at the
// same time, add a random element to the path. Keep it small enough to
// avoid a MAX_PATH violation on Windows.
const auto rand{HexStr(g_rng_temp_path.randbytes(10))};
m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand;
TryCreateDirectories(m_path_root);
Comment on lines +146 to 152

@furszy furszy Nov 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about checking TryCreateDirectories too? And doing this in a loop until it creates a new dir?
Probably we might also need to lock the directory like we do for the custom datadir path.

@maflcko maflcko Nov 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loop

I am not sure that running tests (or the setup of them) in a loop until they pass is a good approach. Generally, when there is an issue, it should be addressed, instead of being silently hidden.

Probably we might also need to lock the directory like we do for the custom datadir path.

Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests.

@kevkevinpal kevkevinpal Nov 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we might also need to lock the directory like we do for the custom datadir path.

Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests

Created this PR, let me know if you would prefer any changes or if you think my approach is incorrect!
#31328

} else {
// Custom data directory
Expand Down