🚀 fanquake merged a pull request: "wallet: batch all individual spkms setup db writes in a single db txn"
(https://github.com/bitcoin/bitcoin/pull/28894)
(https://github.com/bitcoin/bitcoin/pull/28894)
🚀 fanquake merged a pull request: "test: Extends MEMPOOL msg functional test"
(https://github.com/bitcoin/bitcoin/pull/28485)
(https://github.com/bitcoin/bitcoin/pull/28485)
🚀 fanquake merged a pull request: "test: fix v2 transport intermittent test failure (#29002)"
(https://github.com/bitcoin/bitcoin/pull/29006)
(https://github.com/bitcoin/bitcoin/pull/29006)
✅ fanquake closed an issue: "Intermittent test failure in p2p_v2_transport"
(https://github.com/bitcoin/bitcoin/issues/29002)
(https://github.com/bitcoin/bitcoin/issues/29002)
✅ fanquake closed a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)
(https://github.com/bitcoin/bitcoin/pull/29009)
💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420319394)
Hm, as far as I know there's no easy way... I once found a bug like that where a deterministic random generator was used, and the unit test was asserting the exact output of an algorithm given a fixed but random input. On Windows the result was different because there the compiler had chosen a different evaluation order.
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420319394)
Hm, as far as I know there's no easy way... I once found a bug like that where a deterministic random generator was used, and the unit test was asserting the exact output of an algorithm given a fixed but random input. On Windows the result was different because there the compiler had chosen a different evaluation order.
💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420321030)
Updated, thanks
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420321030)
Updated, thanks
💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420321687)
Fixed in https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 thank you @martinus
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420321687)
Fixed in https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 thank you @martinus
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1847041044)
> Did you have steps to reproduce outside of guix?
Not yet. Will get some, and open an issue upstream.
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1847041044)
> Did you have steps to reproduce outside of guix?
Not yet. Will get some, and open an issue upstream.
💬 maflcko commented on issue "fuzz: Left over tmp files when fuzzing with afl++":
(https://github.com/bitcoin/bitcoin/issues/28811#issuecomment-1847041403)
Not sure what to do here. There are some options:
* Forcing a static directory path per fuzzing task (Identified by target name + the CPU it runs on, or the "worker ID"), which will be cleared before the fuzz process starts.
* Detecting a low storage and then aborting the fuzzing campaign with a verbose error message, educating about the timeout?
* (something else?)
I think an early abort makes the most sense, because with frequent timeouts, it doesn't make sense to continue the fuzzing
...
(https://github.com/bitcoin/bitcoin/issues/28811#issuecomment-1847041403)
Not sure what to do here. There are some options:
* Forcing a static directory path per fuzzing task (Identified by target name + the CPU it runs on, or the "worker ID"), which will be cleared before the fuzz process starts.
* Detecting a low storage and then aborting the fuzzing campaign with a verbose error message, educating about the timeout?
* (something else?)
I think an early abort makes the most sense, because with frequent timeouts, it doesn't make sense to continue the fuzzing
...
📝 maflcko reopened a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.
Fix this by detecting them in the fuzz target.
Can be tested by introducing a bug such as:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::ato
...
(https://github.com/bitcoin/bitcoin/pull/29009)
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.
Fix this by detecting them in the fuzz target.
Can be tested by introducing a bug such as:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::ato
...
💬 fanquake commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#issuecomment-1847045128)
Note there was a typo in the commit message. This fixed #29002, not #29009 (which accidently got closed).
(https://github.com/bitcoin/bitcoin/pull/29006#issuecomment-1847045128)
Note there was a typo in the commit message. This fixed #29002, not #29009 (which accidently got closed).
⚠️ maflcko opened an issue: "test: Add TestNode wait_until helper"
(https://github.com/bitcoin/bitcoin/issues/29029)
### Motivation
Currently, inside TestNode, the global `wait_until_helper_internal` must be used, and each time the timeout factor must be passed.
See https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416891272
### Possible solution
It would be nice to have a method `wait_until` that wraps the `wait_until_helper_internal` call, like in other places.
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* Python 3
##
...
(https://github.com/bitcoin/bitcoin/issues/29029)
### Motivation
Currently, inside TestNode, the global `wait_until_helper_internal` must be used, and each time the timeout factor must be passed.
See https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416891272
### Possible solution
It would be nice to have a method `wait_until` that wraps the `wait_until_helper_internal` call, like in other places.
### Useful Skills
* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* Python 3
##
...
💬 maflcko commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1420334764)
Alternatively, it would be good to pass the `timeout` option to `assert_debug_log`, so that it is well-documented that the debug log is used to sync.
(https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1420334764)
Alternatively, it would be good to pass the `timeout` option to `assert_debug_log`, so that it is well-documented that the debug log is used to sync.
💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1847059669)
code review ACK fab46cc
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1847059669)
code review ACK fab46cc
💬 petertodd commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1847064561)
Worth noting that the vast majority of inscriptions are tiny, BRC-20 tokens, with ~50 bytes of data: https://ordiscan.com/inscriptions
The data in this use-case isn't actually significant. Typically ~50WU out of a ~600WU transaction. Even if BRC-20 and similar tokens modify the protocol to not actually put any data in the Bitcoin chain, they *still* would drive fees up significantly due to the transactions themselves.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1847064561)
Worth noting that the vast majority of inscriptions are tiny, BRC-20 tokens, with ~50 bytes of data: https://ordiscan.com/inscriptions
The data in this use-case isn't actually significant. Typically ~50WU out of a ~600WU transaction. Even if BRC-20 and similar tokens modify the protocol to not actually put any data in the Bitcoin chain, they *still* would drive fees up significantly due to the transactions themselves.
🚀 fanquake merged a pull request: "build: Require C++20 compiler"
(https://github.com/bitcoin/bitcoin/pull/28349)
(https://github.com/bitcoin/bitcoin/pull/28349)
⚠️ maflcko opened an issue: "test: Intermittent issue in rpc_net.py"
(https://github.com/bitcoin/bitcoin/issues/29030)
https://drahtbot.space/temp_scratch/rpc_net_1408.tar.xz
```
test 2023-12-08T12:02:27.339000Z TestFramework (INFO): Check getpeerinfo output before a version message was sent
node0 2023-12-08T12:02:27.341389Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:55486
node0 2023-12-08T12:02:27.342184Z [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
test 2023-12-08T12:02:27.346000Z Tes
...
(https://github.com/bitcoin/bitcoin/issues/29030)
https://drahtbot.space/temp_scratch/rpc_net_1408.tar.xz
```
test 2023-12-08T12:02:27.339000Z TestFramework (INFO): Check getpeerinfo output before a version message was sent
node0 2023-12-08T12:02:27.341389Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:55486
node0 2023-12-08T12:02:27.342184Z [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
test 2023-12-08T12:02:27.346000Z Tes
...
💬 maflcko commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420361508)
question: Does it need to be random? `m_path_root` should already be (more) random. If not, it would be good to remove. Also, to reduce the use of `c_str`, which (in other parts of the code) is fragile and can lead to bugs. So `git grep c_str` will be less verbose if unused used of c_str are removed.
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420361508)
question: Does it need to be random? `m_path_root` should already be (more) random. If not, it would be good to remove. Also, to reduce the use of `c_str`, which (in other parts of the code) is fragile and can lead to bugs. So `git grep c_str` will be less verbose if unused used of c_str are removed.
💬 maflcko commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1847079707)
```
e-argument-comment,-warnings-as-errors]
52 | /*m_mempool_limit_bypassed=*/false,
| ^
./kernel/mempool_entry.h:256:51: note: 'from_disconnected_block' declared here
256 | const bool from_disconnected_block, const bool submitted_in_package,
| ^
test/fuzz/poli
...
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1847079707)
```
e-argument-comment,-warnings-as-errors]
52 | /*m_mempool_limit_bypassed=*/false,
| ^
./kernel/mempool_entry.h:256:51: note: 'from_disconnected_block' declared here
256 | const bool from_disconnected_block, const bool submitted_in_package,
| ^
test/fuzz/poli
...