This is a major overhaul of our GitHub Actions workflow file that introduces several cleanups and optimizations, as well as new jobs dedicated to running just the python functional test suite. Below are some of the more notable changes:
- Split off functional tests to their own jobs. Often times we will see a trivial failure in one of the functional tests. Prior to this change, this meant needing to re-run the entire build job again. With this change, we can trigger a re-run of just the functional test suite without needing to re-compile the wallet again. This is handled by creating a minimal archive "artifact" in the build job, and passing it to the new functional test jobs. Credit goes to @Duddino for coming up with this idea and doing much of the initial research and testing!
- Split old configure/build/unittest groupings into their own steps. Instead of exclusively relying on older group macros in a single step, I've created new more descriptive steps to improve readability and responsiveness of the actions report portal. This also helps in seeing where any significant bottle-neck is for a particular job configuration.
- Unify the `apt_get` and `brew_install` matrix config params. These two params have been unified to just `packages` across all matrix configurations.
- No longer cache the sapling params. Since the sapling params files are part of our source tree anyways, caching them in GitHub Actions cache files is redundant and only serves to waste valuable cache storage space.
- Add some basic descriptive commentary for jobs. Self-explanatory.
Note: The minimalist build artifacts are ONLY intended to be used within the GitHub Actions environment and expire after 5 days from their creation. Failing functional tests that are not resolved within this expiry time will necessitate a full re-run of the workflow. Any use of these artifacts outside of GitHub Actions is discouraged and unsupported.
---
Reviewer's note: GitHub will report this PR as not having all the "required checks" as passing as the names/IDs of jobs have been changed. Once this PR is merged it will be then possible to update the repository settings to reflect the new job names/IDs in the "required checks" settings.
ACKs for top commit: 3ba861c64ff35cc420f717b0286714d40457c2f0
Duddino:
ACK 3ba861c64ff35cc420f717b0286714d40457c2f0
panleone:
utACK 3ba861c64ff35cc420f717b0286714d40457c2f0
f5ba2649b014737ce2ec331b75f46cdeb9bde6a4 fix another possible deadlock (ale)
f162f76229b7bd7e4c238df7feff28719ae23d11 fix: fix data race and remove circular dependency (ale)
9207b8fdf55172ccc394c90b8e9b744106700c03 fix data race: LOCK cs before accessing nodeState (ale)
58e5c8c425825dc3f8b0471480cdc5ea1065101c fix: Solve possible deadlock (ale)
Pull request description:
This PR fixes more thread issues that ThreadSanitizer showed:
first commit:
Solve a possible 3-threads deadlock fixed by locking `mempool.cs` before `cs_inventory` (see the comment for the log)
second commit:
solve the trivial data race where `nodeState` is accessed without locking the corresponding mutex
third commit:
solve all the issues that the function `getQuorumNodes` had:
- `it` iterator was accessed without having the lock on `cs_vPendingMasternodes` which was causing a possible data race
- `connman->vNodes` was accessed without locking the mutex `cs_vNodes` (another possible data race)
Solved by using the function `connman->ForEachNode` and removing the useless getter.
fourth commit:
Solve a possible deadlock: lock `cs_vPendingMasternodes` before calling `connmann->ForEachNode(...)`
ACKs for top commit: f5ba2649b014737ce2ec331b75f46cdeb9bde6a4
Liquid369:
tACK f5ba2649b014737ce2ec331b75f46cdeb9bde6a4
Fuzzbawls:
ACK f5ba2649b014737ce2ec331b75f46cdeb9bde6a4
Merge #2924: [Bug] Fix validation interface data race
ddbd4b01549999efa183373e349fc963d0f76aa2 fix BlockStateCatcher data races (ale)
b01c3df1bdd083be2e7723e0361bffe396a9337b let validationinterface take shared_ptr (ale)
Pull request description:
first commit:
Partially backport bitcoin PR https://github.com/bitcoin/bitcoin/pull/18338 which introduce the functions
`RegisterSharedValidationInterface` and `UnregisterSharedValidationInterface`. See that PR why using normal pointers is problematic, and how `shared_ptr` solve the issue
(In a few words the problem is that it can happen that the pointed memory is freed before all signals have been processed, while with shared pointer we are sure that memory will be freed after the last signal is handled)
second commit:
Utilize the new functions to the validation interface `BlockStateCatcher`. In order to keep everywhere the logic unchanged (Registering on creation and Unregistering when the object goes out of scope) I created the wrapper class `BlockStateCatcherWrapper` which contains a `shared_ptr` to `BlockStatecatcher`.
Those two commits solve the following data race where a `BlockStateCatcher` pointer is dereferenced after the pointed memory is freed
```
WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=6423)
Write of size 8 at 0x7fc3d7c2d570 by thread T20:
#0 CValidationInterface::~CValidationInterface() validationinterface.h:75 (pivxd+0xeacac)
#1 BlockStateCatcher::~BlockStateCatcher() util/blockstatecatcher.h:23 (pivxd+0xeacac)
#2 generateBlocks(Consensus::Params const&, CWallet*, bool, int, int, int, CScript*) rpc/mining.cpp:78 (pivxd+0x1c1a98)
#3 generate(JSONRPCRequest const&) rpc/mining.cpp:140 (pivxd+0x1c2215)
...
Merge #2918: [Build] Update m4 macros from upstream
90b1892c5a402537b0eee5197344e6c4c8dbc964 build: AX_BOOST_UNIT_TEST_FRAMEWORK() serial 22 (Fuzzbawls)
92d478cbd8ff7899298649cf7a9b5a1897fdf5b6 build: AX_BOOST_THREAD() serial 33 (Fuzzbawls)
103b802de4e61ed4dbb2b32ae634ea2cc39b96a4 build: AX_BOOST_SYSTEM() serial 20 (Fuzzbawls)
1712d9a7f5a7ad4914c9c3c39f4e9475ddd2f3dd build: AX_BOOST_FILESYSTEM() serial 28 (Fuzzbawls)
583adf64fbd2eeb1af0a7b35481f239331dedfa1 build: AX_BOOST_CHRONO() serial 5 (Fuzzbawls)
7d93e29a7a2ce203283daefc4da451a33da5a61c build: AX_BOOST_BASE() serial 54 (Fuzzbawls)
076fbf407a7ea159c01c65dd4417e0c9db86a48f build: AX_PTHREAD() serial 31 (Fuzzbawls)
8813235b8b8c57326b06b3e7fe53b4c45891026a build: AX_GCC_FUNC_ATTRIBUTE() serial 13 (Fuzzbawls)
3596f0f2699f5024f93a6e36862186af9f4b099a build: AX_CXX_COMPILE_STDCXX() serial 18 (Fuzzbawls)
52a11ff981bd0bc25b1e4c8025103d5083d42758 build: AX_CHECK_PREPROC_FLAG() serial 6 (Fuzzbawls)
3200d3f38d5511d31eb1781a46360383db6e33ee build: AX_CHECK_LINK_FLAG() serial 6 (Fuzzbawls)
fd783608a25d32cb16cbcf1fae3c1dd7ed6c9e61 build: AX_CHECK_COMPILE_FLAG() serial 6 (Fuzzbawls)
Pull request description:
This is the first part in what will be a series of updates to our autotools build system. these commits are simply updating macro scripts from their upstream published versions. the only manual change that i've made is to ensure there is a new line at the bottom of each file.
ACKs for top commit: 90b1892c5a402537b0eee5197344e6c4c8dbc964
panleone:
utACK 90b1892c5a402537b0eee5197344e6c4c8dbc964,
Liquid369:
tACK 90b1892c5a402537b0eee5197344e6c4c8dbc964
f9a6ebddfa27e1678e464a249759cfe1649143d1 fix: solve data race by making nTimeBestReceived atomic (ale)
4331c3007b1211998d74a66e908f2de7c82ec564 refactor: move nTimeBestReceived to CWallet (ale)
Pull request description:
First commit is a small refactor: the global variable `nTimeBestReceived` is now a private member of the `CWallet` class, as it was used only there.
Second commit: fix the following data race by making `nTimeBestReceived` atomic
