💬 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
💬 TheCharlatan commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3064950028)
Guix build (aarch64):
```
9080e91998fa6f384c00a2e3aab6316fdd2174a89a303b0a602880daf515defb guix-build-f49840dd902c/output/aarch64-linux-gnu/SHA256SUMS.part
c4061ae7a239ea148d2423f7414d9dd03cefbdfd25c2142476fc400b57ccfef8 guix-build-f49840dd902c/output/aarch64-linux-gnu/bitcoin-f49840dd902c-aarch64-linux-gnu-debug.tar.gz
878588692759d57a0ebb00f3ceded73c53e0c2d9f53dbd0a83549a22d48c007c guix-build-f49840dd902c/output/aarch64-linux-gnu/bitcoin-f49840dd902c-aarch64-linux-gnu.tar.gz
1ce7c5b093
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3064950028)
Guix build (aarch64):
```
9080e91998fa6f384c00a2e3aab6316fdd2174a89a303b0a602880daf515defb guix-build-f49840dd902c/output/aarch64-linux-gnu/SHA256SUMS.part
c4061ae7a239ea148d2423f7414d9dd03cefbdfd25c2142476fc400b57ccfef8 guix-build-f49840dd902c/output/aarch64-linux-gnu/bitcoin-f49840dd902c-aarch64-linux-gnu-debug.tar.gz
878588692759d57a0ebb00f3ceded73c53e0c2d9f53dbd0a83549a22d48c007c guix-build-f49840dd902c/output/aarch64-linux-gnu/bitcoin-f49840dd902c-aarch64-linux-gnu.tar.gz
1ce7c5b093
...
📝 yuvicc opened a pull request: "[WIP] tracing: lock contention"
(https://github.com/bitcoin/bitcoin/pull/32952)
Currently there is a macro `DEBUG_LOCKCONTENTION` (see developer-notes.md) to enable logging of contention. The disadvantage of this method is that `bitcoind` needs to be especially compiled with that enabled, and when enabled this quickly produces huge log files, and has a relatively large overhead.
This is a rework of PR #25081.
Previously the binary(`bitcoind`) shows multiple [ELF](https://en.wikipedia.org/wiki/Executable_and_Linkable_Format) notes which is problematic as it can bloat t
...
(https://github.com/bitcoin/bitcoin/pull/32952)
Currently there is a macro `DEBUG_LOCKCONTENTION` (see developer-notes.md) to enable logging of contention. The disadvantage of this method is that `bitcoind` needs to be especially compiled with that enabled, and when enabled this quickly produces huge log files, and has a relatively large overhead.
This is a rework of PR #25081.
Previously the binary(`bitcoind`) shows multiple [ELF](https://en.wikipedia.org/wiki/Executable_and_Linkable_Format) notes which is problematic as it can bloat t
...
👍 hebasto approved a pull request: "ci: Avoid cd into build dir"
(https://github.com/bitcoin/bitcoin/pull/32880#pullrequestreview-3012852648)
ACK fa0eca82ec1222ec1c68835ce7acdf9c8c4740ad, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/32880#pullrequestreview-3012852648)
ACK fa0eca82ec1222ec1c68835ce7acdf9c8c4740ad, I have reviewed the code and it looks OK.
💬 TheCharlatan commented on issue "Bitcoin Kernel Library Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-3065032270)
I re-wrote the tracking issue. It now better reflects all the different areas being worked on and also surfaces a lot of the work I have not opened a pull request for.
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-3065032270)
I re-wrote the tracking issue. It now better reflects all the different areas being worked on and also surfaces a lot of the work I have not opened a pull request for.
📝 hebasto converted_to_draft 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
...
👋 hebasto's pull request is ready for review: "build: Explicitly set Qt's `AUTO{MOC,RCC,UIC}` property per target"
(https://github.com/bitcoin/bitcoin/pull/32951)
(https://github.com/bitcoin/bitcoin/pull/32951)
📝 hebasto converted_to_draft 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
...
📝 hebasto opened a pull request: "[POC] ci: Skip compilation when running static code analysis"
(https://github.com/bitcoin/bitcoin/pull/32953)
This PR is a proof of concept for using a compilation database with static analysis tools such as clang-tidy or IWYU. The idea was suggested in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2935293250:
> Shouldn't this be excluded in the tidy CI task as well?
Additionally, this PR makes use of the `codegen` target, following the suggestion in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497:
> It does seem nice to be able to run clang-tidy on generated file
...
(https://github.com/bitcoin/bitcoin/pull/32953)
This PR is a proof of concept for using a compilation database with static analysis tools such as clang-tidy or IWYU. The idea was suggested in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2935293250:
> Shouldn't this be excluded in the tidy CI task as well?
Additionally, this PR makes use of the `codegen` target, following the suggestion in https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2991861497:
> It does seem nice to be able to run clang-tidy on generated file
...
💬 hebasto commented on pull request "doc: Remove build instruction for running `clang-tidy`":
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-3065305742)
> @hebasto Are you going to follow up here?
There are a few thought is https://github.com/bitcoin/bitcoin/pull/32953.
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-3065305742)
> @hebasto Are you going to follow up here?
There are a few thought is https://github.com/bitcoin/bitcoin/pull/32953.
💬 l0rinc commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3065338523)
Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3065338523)
Lightweight code review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5
📝 hebasto opened a pull request: "cmake: Drop no longer necessary "cmakeMinimumRequired" object"
(https://github.com/bitcoin/bitcoin/pull/32954)
The minimum required CMake version is 3.22:https://github.com/bitcoin/bitcoin/blob/6a13a6106e3c1ebe95ba6430184d6260a7b942bd/CMakeLists.txt#L10
(https://github.com/bitcoin/bitcoin/pull/32954)
The minimum required CMake version is 3.22:https://github.com/bitcoin/bitcoin/blob/6a13a6106e3c1ebe95ba6430184d6260a7b942bd/CMakeLists.txt#L10
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202647828)
For reference, this is the rust test I recently wrote: https://github.com/p2pderivatives/rust-bitcoin-coin-selection/blob/3ae36852f4263270e794c444e9fc09172997fc67/src/branch_and_bound.rs#L866
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2202647828)
For reference, this is the rust test I recently wrote: https://github.com/p2pderivatives/rust-bitcoin-coin-selection/blob/3ae36852f4263270e794c444e9fc09172997fc67/src/branch_and_bound.rs#L866