This change will pass at first but will result in silent merge#8
Closed
This change will pass at first but will result in silent merge#8
Conversation
…vations While reviewing the MuSig2 descriptors PR 31244, I realized that the enum `DeriveType` here logically refers to the derive type for ranged descriptors. This became evident to me while going through the implementations of `IsRange` & `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner` function. Initially I got confused by reading `IsRange` translating to `!= DeriveType::NO`, but later realised it specifically referred to the presence of ranged derivations. I propose explicitly mentioning "RANGED" in the values of the `DeriveType` enum would make it easier to parse the descriptors code. This enum is used in one file only - `script/descriptors.cpp`. That's why I explicitly passed it as the argument in the `sed` commands in the script below. -BEGIN VERIFY SCRIPT- sed -i 's/HARDENED\b/HARDENED_RANGED/g' src/script/descriptor.cpp sed -i 's/\bNO\b/NON_RANGED/g' src/script/descriptor.cpp -END VERIFY SCRIPT-
The current test has a couple issues: 1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission 2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present. Fix the bug and add a few assertions to protect against regressions.
When compiling with clang-cl on Windows, `src/util/subprocess.h` emits `-Wunused-private-field` warnings about unused private fields in the `Popen` class.
When compiling with clang-cl on Windows, `src/util/subprocess.h` emits `-Wunused-private-field` warnings about unused private fields in the `Child` class.
Replace the TODO comment in wallet_assumeutxo.py with actual test assertions that verify node and wallet behavior after a restart during assumeutxo background sync. The new tests verify: - Two chainstates exist (background validation not complete) - Background chainstate is still at START_HEIGHT - Snapshot chainstate has synced to at least PAUSE_HEIGHT - Wallets cannot be loaded after restart (expected behavior during background sync because blocks before snapshot are unavailable for rescanning) - Wallet backup from before snapshot height cannot be restored This documents the expected behavior that wallets cannot be loaded after a node restart during assumeutxo background sync, which is an important edge case for users to be aware of. refs bitcoin#28648
For key_exp_index to count correctly for miniscript expressions, KeyParser should hold a reference to it.
Simplifies the callsites for incrementing key_exp_index
Since pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(), hash160(), after(), and older() all are single argument expressions that are parsed immediately, we can use the Expr and Func parsing functions to determine what the arguments of these expressions are, rather than searching for the next closing parentheses. This fixes an issue when pk(), pk_k(), pkh(), and pk_h() include a musig() expression as Expr properly handles nested expressions.
The data embedded is from the latest ASMap file from the asmap-data repository: https://github.com/asmap/asmap-data/blob/main/1755187200_asmap.dat
This can be disabled with -DWITH_EMBEDDED_ASMAP=OFF.
Extend `test-each-commit` to run on every non-head pull request commit.
The PR tip is excluded because it is already covered by other CI jobs.
Runner was changed to a performant cirrus runner.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Pass `--failfast` to the functional test runner in `.github/ci-test-each-commit-exec.py`.
Stop after the first failure to surface the root cause sooner and keep logs readable when testing ancestor commits.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
- The DEFAULT_MIN_RELAY_TX_FEE has been reduced to 100 in v30. This change updates block policy fee estimator to reflect that change.
- This commit deletes a dummy field that is "dummy" and unused.
- This commit also refactors the test the better track confirmed utxos.
Adds test coverage by randomly calling `EmplaceCoinInternalDANGER` in `SimulationTest` to verify it remains correct as we modify it in a future commit. Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Adds `m_dirty_count` member to track the running count of dirty cache entries as follows: * Incremented when entries are marked dirty via `CCoinsCacheEntry::SetDirty` * Decremented when dirty entries are removed or cleaned * Passed through `CoinsViewCacheCursor` and updated during iteration The dirty count is needed because after non-wiping flushes (introduced in bitcoin#28280 and bitcoin#28233), the percentage of dirty entries in the cache may be far below 100%. Using total cache size for flush warnings and disk space checks is therefore misleading. Updates all test code to properly initialize and maintain the dirty count. Co-authored-by: l0rinc <pap.lorinc@gmail.com> Co-authored-by: Andrew Toth <andrewstoth@gmail.com> Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
…ecks Changes flush warnings to use the actual number of dirty entries being written rather than total cache size or memory usage: * Moves warning from `FlushStateToDisk` to `CCoinsViewDB::BatchWrite` so it applies to both regular flushes and `AssumeUTXO` snapshot writes * Changes threshold from `WARN_FLUSH_COINS_SIZE` (1 GiB) to `WARN_FLUSH_COINS_COUNT` (10M entries), approximately equivalent - this also helps with the confusion caused by UTXO size difference on-disk vs in-memory * Moves benchmark logging to `BatchWrite` where the actual disk I/O occurs to make sure AssumeUTXO also warns * Uses dirty count for disk space check (48 bytes per entry estimate) * Removes redundant `changed` counter since `dirty_count` is now tracked This ensures users are warned appropriately even when only a fraction of the cache is dirty, and provides accurate warnings during `AssumeUTXO` loads. Co-authored-by: l0rinc <pap.lorinc@gmail.com>
…InvalidateBlock - there is no functional difference between BLOCK_FAILED_VALID and BLOCK_FAILED_CHILD and it's unnecessary code complexity to correctly categorise them.
Exercise the case where tasks remain pending and verify that the thread calling Stop() participates in draining the queue
Move the operator<< overloads used by BOOST_CHECK_* out of the unit test machinery test/setup_common, into test/util/common.h. And replace the individual per-type ToString() overloads with a single concept-constrained template that covers any type exposing a ToString() method. This is important to not add uint256.h and transaction_identifier.h dependencies to the shared test/util/common.h file. Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Avoid providing the entire unit test framework dependency to tests that only require access to the HasReason utility class. E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp, and script_parse_tests.cpp only require access to HasReason and nothing else.
Submit tasks to a non-started, interrupted, or stopped pool and verify the proper error is always returned.
Useful for debugging issues. Co-authored-by: Matias Furszyfer <matiasfurszyfer@protonmail.com>
The standard and fuzz matrix jobs share the same github.job value (windows-native-dll), so both try to save the vcpkg tools cache with the same key. Since the tools are identical across build types, let them share a single cache entry by restricting the save to the standard job only.
…windows matrix jobs 17a079c ci: fix vcpkg tools cache key collision between windows matrix jobs (will) Pull request description: The standard and fuzz matrix jobs share the same github.job value (windows-native-dll), so both try to save the vcpkg tools cache with the same key. Since the tools are identical across build types, let them share a single cache entry by restricting the save to the standard job only. ACKs for top commit: maflcko: lgtm ACK 17a079c hebasto: ACK 17a079c, this should fix issues like bitcoin#34559 (comment). Tree-SHA512: 2e83846bfc88947b4bc321baa23563e0c093cd4f55f11f8105c2ecf867b78028aa71aa4780f928103b7a9f1f2e3cf72dbb072f05e7925bc1d00069234acf23c9
0a8d303 test: fix test_limit_enforcement_package (Greg Sanders) Pull request description: The current test has a couple issues: 1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission 2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present. Fix the bug and add a few assertions to protect against regressions. ACKs for top commit: bensig: ACK 0a8d303 achow101: ACK 0a8d303 sipa: ACK 0a8d303 Tree-SHA512: 0ba184d82edc5a502e9119a6876e80c4564c876fa51ee39293d47bd30c18bf3ded50fbd2f6f2a3394784fad05d8f6370a90682068b30358b077280abd2477252
Appveyor is not longer used however the test still requires to check for permissions including 666 as otherwise the tests fail on Windows
…sReason dependency cleanup 408d5b1 test: include response body in non-JSON HTTP error msg (Matthew Zipkin) 9dc653b test: threadpool, add coverage for all Submit() errors (furszy) ce2a984 test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc) d9c6769 test: refactor, decouple HasReason from test framework machinery (furszy) dbbb780 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator) 3b7cbca test: ensure Stop() thread helps drain the queue (seduless) ca101a2 test: coverage for queued tasks completion after interrupt (furszy) bf2c607 threadpool: active-wait during shutdown (furszy) e88d274 test: add threadpool Start-Stop race coverage (furszy) 8cd4a43 threadpool: guard against Start-Stop race (furszy) 9ff1e82 test: cleanup, block threads via semaphore instead of shared_future (l0rinc) Pull request description: A few follow-ups to bitcoin#33689, includes: 1) `ThreadPool` active-wait during shutdown: Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively. This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces. 2) Decouple `HasReason` from the unit test framework machinery This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes. 3) Include response body in non-JSON HTTP error messages Straight from pinheadmz [comment](bitcoin#33689 (comment)), it makes debugging CI issues easier. ACKs for top commit: maflcko: review ACK 408d5b1 🕗 achow101: ACK 408d5b1 hodlinator: re-ACK 408d5b1 Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
```
src/common/netif.cpp:137:51: error: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare]
137 | for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/netlink/netlink.h:220:31: note: expanded from macro 'NLMSG_OK'
220 | #define NLMSG_OK(_hdr, _len) NL_ITEM_OK(_hdr, _len, NLMSG_HDRLEN, _NLMSG_LEN)
| ^ ~~~~ ~~~~~~~~~~~~
/usr/include/netlink/netlink.h:203:10: note: expanded from macro 'NL_ITEM_OK'
203 | ((_len) >= _hlen && _LEN_M(_ptr) >= _hlen && _LEN_M(_ptr) <= (_len))
| ~~~~ ^ ~~~~~
1 error generated.
```
Happens on FreeBSD 15.0, with the default compiler (Clang 19).
On FreeBSD 14, `/usr/include/netlink/netlink.h` contains:
```
#define NLMSG_HDRLEN ((int)sizeof(struct nlmsghdr))
```
On FreeBSD 15, `/usr/include/netlink/netlink.h` contains:
```
#define NLMSG_HDRLEN (sizeof(struct nlmsghdr))
```
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
8834e4e test: remove appveyor reference in comment (Max Edwards) Pull request description: Appveyor is not longer used however the test still requires to check for permissions including 666 as otherwise the tests fail on Windows Fixes: bitcoin#32576 ACKs for top commit: maflcko: lgtm ACK 8834e4e hebasto: ACK 8834e4e. Tree-SHA512: 655b44e52da5e0c6c11c79bb4f92c701c6e0e66dce8d7791ccf1d64e4561fe4d1d5f37c1317bead89c88e4d7208a278925168b419482a6be17abf93d0ebc5dfa
…te output 45133c5 doc: clarify `git range-diff` add/delete output (Lőrinc) Pull request description: ### Problem Range diffs in git are useful after PR rebases, but it has an easy-to-misread failure mode: if it cannot match a commit between the old and new ranges, it will show the old commit as removed (<) and the new commit as added (>), without showing any patch contents for that commit. It can look like there were no code changes when in reality the commit was just treated as unrelated and needs full re-review. ### Example ```bash git fetch upstream ff338fd dd76338 git range-diff ff338fd...dd76338 ``` This produced output like: ```patch 1: 0ca4295 = 93: 139aa4b bench: add on-disk `HaveInputs` benchmark 2: 4b32181 < -: ---------- test: add `HaveInputs` call-path unit tests -: ---------- > 94: 277c57f test: add `HaveInputs` call-path unit tests 3: 8c57687 ! 95: c0c94ec dbwrapper: have `Read` and `Exists` reuse `ReadRaw` @@ Metadata ## Commit message ## dbwrapper: have `Read` and `Exists` reuse `ReadRaw` - `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl` (except that it copies the resulting string on success, but that will be needed for caching anyway). + `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl`. ``` Even though the subject matches, there is no diff shown because the commits did not match - the reviewer could think that only the commit message was changed. This should be treated as **unmatched** rather than **unchanged**. If you expected a match, you can try increasing the search effort: ```bash git range-diff --creation-factor=95 ff338fd...dd76338 ``` which would show for example: ```patch 1: 0ca4295 = 93: 139aa4b bench: add on-disk `HaveInputs` benchmark 2: 4b32181 ! 94: 277c57f test: add `HaveInputs` call-path unit tests @@ Commit message The tests document that `HaveInputs()` consults the cache first and that a cache miss pulls from the backing view via `GetCoin()`. + Co-authored-by: Novo <eunovo9@gmail.com> + ## src/test/coins_tests.cpp ## @@ src/test/coins_tests.cpp: BOOST_FIXTURE_TEST_CASE(ccoins_flush_behavior, FlushTest) } } -+BOOST_AUTO_TEST_CASE(ccoins_haveinputs_cache_miss_uses_base_getcoin) ++BOOST_AUTO_TEST_CASE(ccoins_cache_behavior) ``` ### Fix This PR updates `doc/productivity.md` to raise awareness and document this pitfall and mentions `--creation-factor` as a knob to try when the output seems unexpectedly empty. ACKs for top commit: maflcko: review ACK 45133c5 🏦 Sjors: ACK 45133c5 rkrux: crACK 45133c5 sedited: ACK 45133c5 Tree-SHA512: 52dcf6db51425a3ac9789627f80233fb1e3437f7a351acf4a761504d9917837aa1ff8c964605a842ee099fae9842946784f7603f9bffa7051429b2f04b7900be
fa6af85 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke) fa69297 util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke) Pull request description: The call to `tcgetattr` may fail with `ENOTTY`, leaving the struct possibly uninitialized (UB). Fix this UB by returning early when `isatty` fails, or when `tcgetattr` fails. (Same for Windows) This can be tested by a command that fails valgrind before the change and passes after: ``` echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime ACKs for top commit: achow101: ACK fa6af85 l0rinc: lightly tested code review ACK fa6af85 sedited: ACK fa6af85 Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
Normally, when a symlinked test script is executed directly (e.g., `./bld-cmake/test/functional/wallet_disable.py`), Python's default behavior is to resolve the symlink of the script itself, setting `sys.path[0]` to the directory containing the physical source file. Consequently, the test framework util.py is imported from the source tree, and `Path(__file__).parents[3]` correctly resolves to the source root. However, `feature_framework_testshell.py` is unique because it manually inserts `Path(__file__).parent` into `sys.path`. That refers to the build tree and when importing the test framework util.py, the `Path(__file__).parents[3]` will incorrectly point to the build directory instead of the source root. Use `.resolve()` to ensure the Valgrind suppressions file path is always calculated relative to the physical source file, regardless of how the framework was imported.
…tewayImpl() c1361fc netif: fix compilation warning in QueryDefaultGatewayImpl() (MarcoFalke) Pull request description: ``` src/common/netif.cpp:137:51: error: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare] 137 | for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/netlink/netlink.h:220:31: note: expanded from macro 'NLMSG_OK' 220 | #define NLMSG_OK(_hdr, _len) NL_ITEM_OK(_hdr, _len, NLMSG_HDRLEN, _NLMSG_LEN) | ^ ~~~~ ~~~~~~~~~~~~ /usr/include/netlink/netlink.h:203:10: note: expanded from macro 'NL_ITEM_OK' 203 | ((_len) >= _hlen && _LEN_M(_ptr) >= _hlen && _LEN_M(_ptr) <= (_len)) | ~~~~ ^ ~~~~~ 1 error generated. ``` Happens on FreeBSD 15.0, with the default compiler (Clang 19). On FreeBSD 14, `/usr/include/netlink/netlink.h` contains: ``` #define NLMSG_HDRLEN ((int)sizeof(struct nlmsghdr)) ``` On FreeBSD 15, `/usr/include/netlink/netlink.h` contains: ``` #define NLMSG_HDRLEN (sizeof(struct nlmsghdr)) ``` ACKs for top commit: maflcko: lgtm ACK c1361fc hebasto: ACK c1361fc. Tree-SHA512: f8f00e2106fdf91550ab388a65bb8408fcf8c4557da9614e2e1dd90e70fc010dff72bfabbbec4315335afdedb546f7b554f5c98133c5aa1d3077c5641d94b956
…cirrus runner switch fa18be2 test: Fix typo (MarcoFalke) fac9326 ci: Set TEST_RUNNER_PORT_MIN in test-each after cirrus runner switch (MarcoFalke) Pull request description: This is needed after the recent switch to cirrus runners in the task in commit c8c9c1e. Otherwise, the CI will fail: ``` node1 stderr Error: Unable to bind to 127.0.0.1:12321 on this computer. Bitcoin Core is probably already running. ``` (https://github.com/bitcoin/bitcoin/actions/runs/22398358349/job/64837827234?pr=31723#step:9:2605) Also, include a random second commit, so that the CI task is run in this pull. ACKs for top commit: l0rinc: ACK fa18be2 willcl-ark: ACK fa18be2 Tree-SHA512: 6b63f645bf62d3e951ca155cddf3dc562b7ce675ccae4f9179e2202679685b5c147844eb350bd219b173fe2bb976376d0caa073d3e827a48c13aa015f4745b2c
…e for test shell fab51e4 test: Move valgrind.supp to the other sanitizer_suppressions files (MarcoFalke) fa9cf81 test: Add missing resolve() to valgrind.supp file (MarcoFalke) Pull request description: (see commit message for details) Can be tested via: ``` ./bld-cmake/test/functional/feature_framework_testshell.py --valgrind ``` Before: ``` bitcoind exited with status 1 during initialization. ==1735813== FATAL: can't open suppressions file "/b-c/b-c-2/bld-cmake/contrib/valgrind.supp" ``` After: (passes) Also, move the suppression file to all the other suppression files. ACKs for top commit: frankomosh: Re-ACK fab51e4 Tree-SHA512: d3e3e130c0e2292ca3ab9e80d2ebec6b4edc7914280ed90fb4af8a65cd1c9edd19064d86375a6787b864925fe0e47bab2321f89b9be8d1613a0aebf4ec2443fe
fa36ade ci: [refactor] Drop last use of pwsh (MarcoFalke) fae31b1 ci: [refactor] Move github_import_vs_env to python script (MarcoFalke) Pull request description: The use of pwsh was a bit confusing and inconsistent around the exit code. See also bitcoin#32672 I think it is fine to drop it and purely use Bash/Python. This also moves a bit of code from the github yaml to the python script. ACKs for top commit: m3dwards: Looks good! re-ACK bitcoin@fa36ade hodlinator: re-ACK fa36ade Tree-SHA512: 78edffc60c58c476b0acca5224150169d154b0b818114844a04295af9ba19b7cdf1fb2afb738f6cafd6172f0f477d546018ebf95061eb5bd8bbb35e065a129d4
…' failed`` errors on early startup bbc8f1e ipc mining: Prevent ``Assertion `m_node.chainman' failed`` errors on early startup (Ryan Ofsky) a7cabf9 init refactor: Only initialize node.notifications one time (Ryan Ofsky) c8e332c init refactor: Remove node.init accesss in AppInitInterfaces (Ryan Ofsky) Pull request description: This fixes ``Assertion `m_node.chainman' failed`` errors first reported bitcoin#33994 (comment) when IPC mining methods are called before ChainstateManager is loaded. The fix works by making the `Init.makeMining` method wait until chainstate data is loaded. It's probably the simplest possible fix but other alternatives like moving the wait to `Mining.createNewBlock` were discussed in the thread bitcoin#34661 (comment) and could be implemented later without changes to clients. ACKs for top commit: Sjors: utACK bbc8f1e ismaelsadeeq: ACK bbc8f1e achow101: ACK bbc8f1e Tree-SHA512: 3e2e4e28ccff364b2303efd06ce337a229c28609076638500acb29559f716a15ad99409c6970ce9ad91776d53e3f9d959f1bbbd144ea9a4a2fb578ddbf2da267
fa48f8c test: Add missing timeout_factor to zmq socket (MarcoFalke) Pull request description: Fixes bitcoin#34189 Otherwise, the test may intermittently fail on slow CI systems that have `--timeout-factor=` properly set. It can be tested by running `./bld-cmake/test/functional/interface_zmq.py --timeout-factor=10` with this diff: ```diff diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index c7be6ab..b14cf2aee6 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -166,2 +166,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue() LOG_EVENT(fmt, local_name, __VA_ARGS__); \ + UninterruptibleSleep(45ms); \ event(); \ diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py index 6717007..eee377daea 100755 --- a/test/functional/interface_zmq.py +++ b/test/functional/interface_zmq.py @@ -176,3 +176,3 @@ class ZMQTest (BitcoinTestFramework): for sub in subscribers: - sub.socket.set(zmq.RCVTIMEO, recv_timeout*1000) + sub.socket.set(zmq.RCVTIMEO, int(recv_timeout * 1000)) @@ -271,3 +271,3 @@ class ZMQTest (BitcoinTestFramework): [(topic, address) for topic in ["hashblock", "hashtx"]], - recv_timeout=2) # 2 second timeout to check end of notifications + recv_timeout=0.2) # 2 second timeout to check end of notifications self.disconnect_nodes(0, 1) ``` Before this pull: Test fails with `zmq.error.Again: Resource temporarily unavailable` After this pull: Test passes ACKs for top commit: l0rinc: code review ACK fa48f8c achow101: ACK fa48f8c Tree-SHA512: 5ff8bdd807ff4b644daa634bb7b469da3da3915f58afba63a90e662df99cbebc86636e34e2b1b313c8629773aef2a239fb3025226a84d2ec22f6ecd4cea666c4
…_private_broadcast.py da7f70a test: use port 0 for I2P addresses in p2p_private_broadcast.py (Vasil Dimov) a8ebcfd test: let connections happen in any order in p2p_private_broadcast.py (Vasil Dimov) 67696b2 net: extend log message to include attempted connection type (Vasil Dimov) Pull request description: If the following two events happen: * (likely) the automatic 10 initial connections are not made to all networks * (unlikely) the network-specific logic kicks in almost immediately. It is using exponential distribution with a mean of 5 minutes (`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`). So if both happen, then the 11th connection may not be the expected private broadcast, but a network-specific connection. Fix this by retrieving the connection type from `destinations_factory()`. This is more flexible because it allows connections to happen in any order and does not break if e.g. the 11th connection is not the expected first private broadcast. This also makes the test run faster: before: 19-44 sec now: 10-25 sec because for example there is no need to wait for the initial 10 automatic outbound connections to be made in order to proceed. Fixes: bitcoin#34387 ACKs for top commit: achow101: ACK da7f70a andrewtoth: ACK da7f70a mzumsande: Code Review ACK da7f70a Tree-SHA512: 7c293e59c15c148a438e0119343b05eb278205640658c99336d4caf4848c5bae92b48e15f325fa616cbc9d5f394649abfa02406a76e802cffbd3d312a22a6885
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR should be fine as a PR but when it's merged into master it will cause a silent merge conflict with #5