-
Notifications
You must be signed in to change notification settings - Fork 84
refactor(tests): Improve performance when obtaining bitcoind binary
#716
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.
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
refactor(tests): Improve performance when obtaining bitcoind binary
#716
Conversation
|
Concept ACK. Just two things
|
qlrd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cACK
Just a minor thing about the path of binaries
I agree the script is getting more complex, but the real issue is how we obtain the bitcoind binary. Building Bitcoin manually is heavy, requires git cloning and compiling, and wastes time for both developers and CI. I think we should keep only two options: letting the user provide a path to their own bitcoind binary, or downloading a known binary we support. Compiling inside the script brings no real benefit, adds error-handling complexity, and creates compatibility issues with older versions that don’t support CMake. With these two options we cover all use cases without increasing script complexity.
I see these commit-tagged directories more as a workaround than something that actually solves the real problems. The issues come from the tests or the code itself. One reason this appears to work is that every branch change or commit creates a new log path, so test functions that read logs avoid breaking because the logs stay small. But this is already being fixed in PR 671, which reduces log size and prevents those failures. Outside of that, using commit tags for directories doesn’t add real value. |
8eb0f6d to
d78af70
Compare
The decision to use git tags that i made when i wrote this We have a directory to store the results of the tests and we need to have some kind of organization that help us to avoid conflicts and to accompany the changes. Random numbers do accomplish this but they arent meaningfull at all and you dont have any way to predict them (because they are random) and thats why i used git tags, they have more utility, please keep them. The real upgrades being introduced here doesnt conflict with git tags at all. |
This isn’t really feasible. Tests that mine blocks break determinism because the block timestamps always differ, so the reproduced results won’t match. And even conceptually, hashing these outputs doesn’t add much value. The purpose of the tests is to validate behavior, and they should pass consistently everywhere.
The upgrade indeed improves performance for CI and for local runs. Keeping the test directories static improves it even further for local use, since you no longer need to rebuild Utreexod or re-download bitcoind every time you switch branches or make a commit. The only thing that needs rebuilding is Floresta itself, which is exactly what should happen. This gives a significant boost for local workflows. |
I think you already have noted that our tests arent ideal and expecting them to work in an ideal setup is logically broken. If the concern is to be ideal, performatic etc etc why are you even considering a good approach to literally mine a block during tests ? Theres a lot of work to do in order to not have any contradictions in how we are doing tests but right now is doing the job that its needing to be done... Introducing too much changes expecting for it to fit an ideal state will only create a development burden for something thats not the focus point of the project, the project is not even released... We need to do an incremental work without breaking too many things and unecessary changes that doesnt really bring any usefull upgrade at all. |
They will never be that deterministic, and I don't think that's necessary. We can reproduce errors without needing perfect determinism. The reason why I liked the tags was so you could keep multiple runs saved on-disk and revisit them as needed. I actually keep logs for different runs when I'm developing something new. Perhaps we can leave the root folder and |
Its not that necessary but IMHO we should keep it in mind, I dont think is any good to have tests that differ in results... Certainly theres a way to accomplish a certain level of certainty of its results.
Yeah, proposed that in #716 (comment) |
e1beb1a to
49dc846
Compare
|
@jaoleal @Davidson-Souza I set up the paths for the logs based on |
|
I ran the script some times locally, its pretty good! |
Its cool to have the possibility to share states between tests cc @j-moreno-c-r |
49dc846 to
a390282
Compare
Removing the |
|
I removed downloading the essential Bitcoin Core components. Now it only installs |
|
Needs rebase |
a390282 to
a7e43e5
Compare
|
I did the rebase. |
6874013 to
598db3e
Compare
|
Version |
598db3e to
2bfa7bb
Compare
|
I made adjustments to the code. Instead of checking hashes that are already in |
They now have a |
|
In 3a4c028 commit message you say: |
|
Also: for versions > 30, can we disable |
|
I ran with the build from source option, and even though the Bitcoin Core build failed, it kept going. I think it should abort in this case |
2bfa7bb to
9a21743
Compare
|
I updated the default bitcoin version to |
9a21743 to
a02623a
Compare
- Introduced `BINARIES_DIR` and `BUILD_DIR` variables to centralize the location of binaries and build artifacts, making it easier to manage and reuse across builds. - Changed the default Bitcoin Core version from `29.0` to `30.2`. - Made the binaries directory deterministic instead of random, allowing reuse of downloaded or built binaries across runs, avoiding unnecessary re-downloads or rebuilds. - Implemented two modes for obtaining the `bitcoind` binary: 1. Using a user-provided binary via `BITCOIND_EXE`. 2. Downloading prebuilt binaries for supported versions and platforms. 3. Building from source as a last resort. - Improved the handling of prebuilt binaries by adding SHA256 checksum validation and support for specific versions (`30.2`, `29.2`, `28.3`, `27.2`). - Updated the `running-tests.md` documentation to reflect the changes in `prepare.sh`, including the new default version, deterministic binaries directory, and the three methods for obtaining `bitcoind`.
- Changed the default build mode for Floresta to debug. To build in release mode, the `--release` flag must now be explicitly passed to the `prepare.sh` script. - Updated the `running-tests.md` documentation to explain the `--release` flag and its usage, including how it optimizes the build for production.
- Removed installation of Bitcoin Core dependencies. - Added required dependencies for compiling the project with Bitcoin Kernel support.
a02623a to
0ec125c
Compare
|
ACK 0ec125c |
Davidson-Souza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 0ec125c
Description and Notes
This PR refactors
./test/prepare.shto significantly improve performance when obtainingbitcoind. Instead of always cloning and building Bitcoin Core, the script now:bitcoindpath.This reduces build time during local development and CI. Functional tests currently take about 12 minutes, and with these changes the execution time should drop to roughly 8 minutes.
The script also now uses a deterministic binaries directory instead of generating a new path per commit. Since only Floresta changes on each run and
prepare.shalways rebuilds it, a static path under/tmp/floresta-func-testsmakes the process faster and more consistent.Another change is that the default build mode for Floresta is now debug. Release builds should be explicitly triggered with the
--releaseflag. This avoids expensive build times when iterating on tests locally while still allowing CI and production-level testing to use optimized builds. All updates are documented.How to verify the changes you have done?
# The script should skip downloading or building Bitcoin Core and use the provided binary. BITCOIND_EXE=/path/to/bitcoind ./tests/prepare.sh# A prebuilt binary should be downloaded if the version and platform are supported. BITCOIN_REVISION=28.3 ./tests/prepare.sh# If no prebuilt binary exists, the script should clone and build Bitcoin Core automatically. BITCOIN_REVISION=28.0 BUILD_BITCOIND_NPROCS=2 ./tests/prepare.sh