💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3063912248)
> Are you running this on an older machine, or are missing some of the crypto accelerator features? Your reported runtimes are about an order of magnitude longer than mine and even our CI machines report the tests running much faster.
Hmm, it's a bit old I guess; 2019-era high-end desktop cpu, and running in a vm under qemu. It's also a debug build, with the extra checks that implies.
CI failures seem to indicate my cmake code doesn't work with something other than ninja as the test conduc
...
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3063912248)
> Are you running this on an older machine, or are missing some of the crypto accelerator features? Your reported runtimes are about an order of magnitude longer than mine and even our CI machines report the tests running much faster.
Hmm, it's a bit old I guess; 2019-era high-end desktop cpu, and running in a vm under qemu. It's also a debug build, with the extra checks that implies.
CI failures seem to indicate my cmake code doesn't work with something other than ninja as the test conduc
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201921789)
```diff
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 58ee99e10bf..b824b6b939e 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -409,11 +409,11 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage)
FUZZ_TARGET(txorphanage_sim)
{
SeedRandomStateForTest(SeedRand::ZEROS);
- // This is a comphehensive simulation fuzz test, which runs through a scenario involving up to 16 transactions
- // (which may have simple
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2201921789)
```diff
diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 58ee99e10bf..b824b6b939e 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -409,11 +409,11 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage)
FUZZ_TARGET(txorphanage_sim)
{
SeedRandomStateForTest(SeedRand::ZEROS);
- // This is a comphehensive simulation fuzz test, which runs through a scenario involving up to 16 transactions
- // (which may have simple
...
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2201944039)
I don't think we need it either, for the history we could still browse back this PR in the repo.
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2201944039)
I don't think we need it either, for the history we could still browse back this PR in the repo.
🤔 hebasto reviewed a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012113611)
My another Guix build:
```
x86_64
f73822833d8c3ff2b2dda2ddc5e16cb1794aa4eb299b1f468fa8dbadfd6b2310 guix-build-8f766f39df3e/output/aarch64-linux-gnu/SHA256SUMS.part
35783f79defa8b4032b78443a28e88a9c11314470fc177afa9e21d0e613bd937 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu-debug.tar.gz
b0345a28e82514d51cb363d798d950e44e630ba357a5e522537e1a7b4a37a151 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu.tar.gz
e
...
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012113611)
My another Guix build:
```
x86_64
f73822833d8c3ff2b2dda2ddc5e16cb1794aa4eb299b1f468fa8dbadfd6b2310 guix-build-8f766f39df3e/output/aarch64-linux-gnu/SHA256SUMS.part
35783f79defa8b4032b78443a28e88a9c11314470fc177afa9e21d0e613bd937 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu-debug.tar.gz
b0345a28e82514d51cb363d798d950e44e630ba357a5e522537e1a7b4a37a151 guix-build-8f766f39df3e/output/aarch64-linux-gnu/bitcoin-8f766f39df3e-aarch64-linux-gnu.tar.gz
e
...
👍 hebasto approved a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012133349)
ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338, tested on Ubuntu 24.04.
(https://github.com/bitcoin/bitcoin/pull/32937#pullrequestreview-3012133349)
ACK 8f766f39df3e312f79f461b2accc2f9c90fa6338, tested on Ubuntu 24.04.
🚀 hebasto merged a pull request: "Enable `-Werror=dev` in CI & Guix"
(https://github.com/bitcoin/bitcoin/pull/32937)
(https://github.com/bitcoin/bitcoin/pull/32937)
📝 hebasto opened a pull request: "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target"
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
(https://github.com/bitcoin/bitcoin/pull/32951)
The codebase, which is compiled, consists of both files in the source tree and additional files generated during the build process. These generated files include headers such as `bitcoin-build-config.h`, headers generated for tests and benchmarks from data files, and files produced by Qt's tools, such as `moc`, `rcc` and `uic`.
When using Makefile or Ninja generators, CMake 3.31 and later provides a [convenient](https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497) builtin b
...
🤔 hodlinator reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-3010366559)
Light post-merge ACK
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-3010366559)
Light post-merge ACK
💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201918072)
nit: Missing `AssertLockHeld` in pre-existing `TxId` overload?
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201918072)
nit: Missing `AssertLockHeld` in pre-existing `TxId` overload?
💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201874252)
nit: There's 7 extra spaces in function prototypes for `GetRequestable` (this is 1 of 3 occurrences). This is due to changes to the code before the scripted-diff not accounting for a loss of `Variant` from the previous lines return value.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201874252)
nit: There's 7 extra spaces in function prototypes for `GetRequestable` (this is 1 of 3 occurrences). This is due to changes to the code before the scripted-diff not accounting for a loss of `Variant` from the previous lines return value.
💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201848886)
Thanks for taking this suggestion, including renaming the field!
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201848886)
Thanks for taking this suggestion, including renaming the field!
💬 hebasto commented on pull request "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target":
(https://github.com/bitcoin/bitcoin/pull/32951#issuecomment-3064055257)
My Guix build:
```
aarch64
6ab7ae50b219efd4675878dabfc783e0335bc67af31ad8bdfeb4d6317b66a054 guix-build-d38eca989aeb/output/aarch64-linux-gnu/SHA256SUMS.part
0ab47d80a7cc03eb654c5ad3b80b7a7ca7f0e02a92b072f21a395ff2508c1ee1 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu-debug.tar.gz
5fb1183124482b3c49411b8981743e9dba2c959b935a67e7ca938f5aad677ba3 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu.tar.gz
3eb65847
...
(https://github.com/bitcoin/bitcoin/pull/32951#issuecomment-3064055257)
My Guix build:
```
aarch64
6ab7ae50b219efd4675878dabfc783e0335bc67af31ad8bdfeb4d6317b66a054 guix-build-d38eca989aeb/output/aarch64-linux-gnu/SHA256SUMS.part
0ab47d80a7cc03eb654c5ad3b80b7a7ca7f0e02a92b072f21a395ff2508c1ee1 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu-debug.tar.gz
5fb1183124482b3c49411b8981743e9dba2c959b935a67e7ca938f5aad677ba3 guix-build-d38eca989aeb/output/aarch64-linux-gnu/bitcoin-d38eca989aeb-aarch64-linux-gnu.tar.gz
3eb65847
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202009761)
> We already have the [coinselection_bnb fuzz target](https://github.com/bitcoin/bitcoin/blob/23e15d40b96a23112fba24cece5fe513ae1a2877/src/wallet/test/fuzz/coinselection.cpp#L347C1-L351C1) which runs BnB on UTXO pools of up to 10,000 UTXOs and validates that if BnB finds a solution that it falls in the solution window.
I see. That is also valuable. However, this does not test the case where a solution does exist (say with 20 UTXOS) and BnB does not find the result, only that any found solut
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202009761)
> We already have the [coinselection_bnb fuzz target](https://github.com/bitcoin/bitcoin/blob/23e15d40b96a23112fba24cece5fe513ae1a2877/src/wallet/test/fuzz/coinselection.cpp#L347C1-L351C1) which runs BnB on UTXO pools of up to 10,000 UTXOs and validates that if BnB finds a solution that it falls in the solution window.
I see. That is also valuable. However, this does not test the case where a solution does exist (say with 20 UTXOS) and BnB does not find the result, only that any found solut
...
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202046274)
Let’s say you create a fuzz target that generates two sets of UTXOs and sums up the effective value of one set to set that as its target. For UTXO counts exceeding 17, BnB might return a solution and then we could check exactly the properties that `coinselection_bnb` checks.
However, while #32150 introduces the use of `m_algo_completed` and `m_selections_evaluated` to BnB, they are properties of `SelectionResult`—when a coin selection algorithm fails to find a solution, it does not return a `
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202046274)
Let’s say you create a fuzz target that generates two sets of UTXOs and sums up the effective value of one set to set that as its target. For UTXO counts exceeding 17, BnB might return a solution and then we could check exactly the properties that `coinselection_bnb` checks.
However, while #32150 introduces the use of `m_algo_completed` and `m_selections_evaluated` to BnB, they are properties of `SelectionResult`—when a coin selection algorithm fails to find a solution, it does not return a `
...
💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202048060)
It may however be interesting to introduce another Error class to coin selection which surfaces when the iteration limit was hit before a solution was found. In that case, we could distinguish between not finding a solution due to exhausting iterations and missing solutions, and running BnB with bigger UTXO pools could be interesting.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202048060)
It may however be interesting to introduce another Error class to coin selection which surfaces when the iteration limit was hit before a solution was found. In that case, we could distinguish between not finding a solution due to exhausting iterations and missing solutions, and running BnB with bigger UTXO pools could be interesting.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202233279)
> It may however be interesting to introduce another Error class to coin selection which surfaces when the iteration limit was hit before a solution was found
Yes exactly. That's how the test works in the rust implementation I wrote. A solution is created from one of two pools and then the pools are combined and shuffled. I have an error that is returned that distinguish iteration limit reached from no solution, so to test BnB is correct, we know we should never see no solution, only itera
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202233279)
> It may however be interesting to introduce another Error class to coin selection which surfaces when the iteration limit was hit before a solution was found
Yes exactly. That's how the test works in the rust implementation I wrote. A solution is created from one of two pools and then the pools are combined and shuffled. I have an error that is returned that distinguish iteration limit reached from no solution, so to test BnB is correct, we know we should never see no solution, only itera
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202239602)
I did not mean to use two UTXO sets. If you have a single UTXO set and one swaps the long_term_fee_rate and the short term fee_rate, that ought to produce either the same results or different results depending on if the current fees are expensive or if the current fee_rates are cheap. I think the test you linked to shows that since the same set is used `{2, 3, 5, 10}` which returns different results depending on the difference in fee_rate and long_term_fee_rate. The difference between expensi
...
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202239602)
I did not mean to use two UTXO sets. If you have a single UTXO set and one swaps the long_term_fee_rate and the short term fee_rate, that ought to produce either the same results or different results depending on if the current fees are expensive or if the current fee_rates are cheap. I think the test you linked to shows that since the same set is used `{2, 3, 5, 10}` which returns different results depending on the difference in fee_rate and long_term_fee_rate. The difference between expensi
...
💬 ariard commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3064667599)
> I agree this needs a ML post. Will take care of this tomorrow.
Answered back to the latest post of the PR author on the ML.
I do think it’s introducing a new DoS vector for multi-party construction, as now one of your prospective counterparty in the flow can maliciously contribute an input hitting this policy limit. The lightning flow should be safe as it’s sanitizing out non-segwit inputs (here for [`tx_add_input`](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-t
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3064667599)
> I agree this needs a ML post. Will take care of this tomorrow.
Answered back to the latest post of the PR author on the ML.
I do think it’s introducing a new DoS vector for multi-party construction, as now one of your prospective counterparty in the flow can maliciously contribute an input hitting this policy limit. The lightning flow should be safe as it’s sanitizing out non-segwit inputs (here for [`tx_add_input`](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-t
...
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3064763860)
update: rebased to master!
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3064763860)
update: rebased to master!
🤔 BrandonOdiwuor reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-3012841109)
Tested ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
Tested on macOS 15.5
Was able to connect `bitcoin-cli` to `bitcoin-node` successfully over `ipc`
<img width="837" height="453" alt="Screenshot 2025-07-12 at 11 24 10" src="https://github.com/user-attachments/assets/179360fd-5572-44c0-b581-6afe50e7223c" />
- Also confirmed that `bitcoin-cli` uses `IPC` as the preferred mode of connection over `HTTP` even when the `-ipcconnect` option is not provided
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-3012841109)
Tested ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
Tested on macOS 15.5
Was able to connect `bitcoin-cli` to `bitcoin-node` successfully over `ipc`
<img width="837" height="453" alt="Screenshot 2025-07-12 at 11 24 10" src="https://github.com/user-attachments/assets/179360fd-5572-44c0-b581-6afe50e7223c" />
- Also confirmed that `bitcoin-cli` uses `IPC` as the preferred mode of connection over `HTTP` even when the `-ipcconnect` option is not provided