test: Revert to random path element #31317

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2411-test-more-rand changing 1 files +13 −4
  1. maflcko commented at 1:27 pm on November 18, 2024: member

    The randomness in the path element is required to allow a single fuzz test to run in parallel. Previous releases used a uint256 random value, but 10 random bytes should be sufficient as well, while avoiding a MAX_PATH violation on Windows.

    The issue was introduced by myself, by suggesting to use the current time in #31000 (review).

  2. test: Revert to random path element fa80b08fef
  3. DrahtBot commented at 1:27 pm on November 18, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31317.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, tdb3, kevkevinpal, dergoegge
    Stale ACK furszy

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31328 (test: locking -testdatadir when not specified and then deleting lock and dir at end of test by kevkevinpal)
    • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)

    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.

  4. DrahtBot added the label Tests on Nov 18, 2024
  5. maflcko commented at 1:33 pm on November 18, 2024: member

    This should fix https://issues.oss-fuzz.com/issues/379418970, which seems to be running on servers that do not have nanosecond precision:

    0libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove_all: No such file or directory ["/tmp/test_common bitcoin/1731742732339168000/regtest/fuzzed_i2p_private_key"]
    

    To test locally, one can reduce to microsecond precision (or less) via:

     0diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
     1index 46a6daa88c..08aabbd026 100644
     2--- a/src/test/util/setup_common.cpp
     3+++ b/src/test/util/setup_common.cpp
     4@@ -138,7 +138,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
     5 
     6     const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
     7     if (!m_node.args->IsArgSet("-testdatadir")) {
     8-        const auto now{TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())};
     9+        const auto now{TicksSinceEpoch<std::chrono::microseconds>(SystemClock::now())};
    10         m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / util::ToString(now);
    11         TryCreateDirectories(m_path_root);
    12     } else {
    

    Then running something like FUZZ=utxo_total_supply ./bld-cmake/src/test/fuzz/fuzz -runs=10 -jobs=2 -workers=2 should eventually crash.

  6. maflcko added this to the milestone 29.0 on Nov 18, 2024
  7. in src/test/util/setup_common.cpp:149 in fa80b08fef outdated
    146+        // tests, such as the fuzz tests to run in several processes at the
    147+        // same time, add a random element to the path. Keep it small enough to
    148+        // avoid a MAX_PATH violation on Windows.
    149+        const auto rand{HexStr(g_rng_temp_path.randbytes(10))};
    150+        m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / rand;
    151         TryCreateDirectories(m_path_root);
    


    furszy commented at 3:33 pm on November 18, 2024:
    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 commented at 3:50 pm on November 18, 2024:

    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 commented at 2:06 am on November 20, 2024:

    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! https://github.com/bitcoin/bitcoin/pull/31328

  8. furszy commented at 3:57 pm on November 18, 2024: member
    Code ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
  9. tdb3 approved
  10. tdb3 commented at 5:19 pm on November 18, 2024: contributor

    code review and light test ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3

    Ran unit tests and benchmarks, and saw that the dirs generated were random.

  11. kevkevinpal commented at 10:09 pm on November 18, 2024: contributor

    tACK fa80b08

    was able to reproduce the crash from #31317 (comment) locally on my machine. Reviewed and ran the code aswell and it looks good to me

  12. in src/test/util/setup_common.cpp:78 in fa80b08fef outdated
    74@@ -75,6 +75,8 @@ using node::VerifyLoadedChainstate;
    75 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
    76 
    77 constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
    78+/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
    


    hodlinator commented at 1:51 pm on November 19, 2024:

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

    Ran this ~10 times:

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

    Many of the failures come down to:

    0Done 2 runs in 0 second(s)
    1terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
    2  what():  filesystem error: cannot remove all: No such file or directory [/tmp/test_common bitcoin/02ccd0019e3e40fb6001] [/tmp/test_common bitcoin/02ccd0019e3e40fb6001/regtest]
    3==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 97e16f57042cab07e5e73f6bed19feec2006e4f7, 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).
    0/** Random context to get unique temp data dirs. */
    

    maflcko commented at 4:07 pm on November 19, 2024:

    Yes, this issue is re-introduced.

    It can also be tested on top of https://github.com/bitcoin/bitcoin/commit/fa80b08fef0eaa600339caa678fdf80a8aec3ce3 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 commented at 10:46 pm on November 19, 2024:

    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 commented at 7:16 am on November 20, 2024:

    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 commented at 10:26 am on November 20, 2024:
    I like the robustness of the current solution. Never know where SeedRandomForTest might creep in inside unit/bench/fuzz. Please resolve this.
  13. hodlinator changes_requested
  14. hodlinator commented at 1:52 pm on November 19, 2024: contributor
    Reviewed fa80b08fef0eaa600339caa678fdf80a8aec3ce3
  15. maflcko force-pushed on Nov 19, 2024
  16. test: Make g_rng_temp_path rand, not dependent on SeedRandomForTest faaaf59f71
  17. maflcko force-pushed on Nov 19, 2024
  18. hodlinator approved
  19. hodlinator commented at 10:47 pm on November 19, 2024: contributor

    ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2

    Tested cherrypicking 8e5f8a4f775b93d30b86d28405886eea232cf875 on top and re-running test from https://github.com/bitcoin/bitcoin/pull/31317/files#r1848404959, confirming that the second commit does in fact make paths random.

  20. DrahtBot requested review from tdb3 on Nov 19, 2024
  21. DrahtBot requested review from furszy on Nov 19, 2024
  22. tdb3 approved
  23. tdb3 commented at 1:22 am on November 20, 2024: contributor

    re ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2

    re-ran unit tests and benchmarks, and saw that the dirs generated were random.

  24. DrahtBot requested review from kevkevinpal on Nov 20, 2024
  25. kevkevinpal commented at 2:16 am on November 20, 2024: contributor

    reACK https://github.com/bitcoin/bitcoin/commit/faaaf59f71ede057b2c1d369ef8db973c2f2dbc2

    Ran fuzz tests with 8 jobs and 8 workers FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=8 -workers=8 and validated that the temp directorys at /tmp/test_common bitcoin/<random value> got created and deleted for each fuzz test.

  26. dergoegge approved
  27. dergoegge commented at 9:49 am on November 20, 2024: member
    ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2
  28. fanquake merged this on Nov 20, 2024
  29. fanquake closed this on Nov 20, 2024

  30. maflcko deleted the branch on Nov 20, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me