💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649284322)
This is a failing test to show BnB is producing change.

⚠️ darosior opened an issue: "`rpc_getblockstats.py` fails with `--gen-test-data`"
(https://github.com/bitcoin/bitcoin/issues/31838)
The test only pass with the currently generated data. It will fail when using the integrated feature to generate this data and running against it.

Reproduction instructions. On top of current master 1d813e4bf52a207ec526303df7b2e3d40d5eaa54 create a clean default build and run the test:
```
cmake -B repro
cmake --build repro -j20
./repro/test/functional/rpc_getblockstats.py --gen-test-data
```

Initially it will fail because it's missing the wallet arguments. Patch it with:
```diff
diff --git a/
...

💬 yancyribbens commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649290842)
> This is a failing test to show BnB is producing change.

With those parameters though, BnB should be producing change.

👍 instagibbs approved a pull request: "test: add missing sync to p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/31837#pullrequestreview-2607241569)
ACK 8fe552fe6e0f1fb3600d4c5bc53b4873def6e94e

💬 instagibbs commented on pull request "test: add missing sync to p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1949936893)
could also `send_and_ping`

🤔 sipa reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2607272614)
Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.

💬 theuni commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#issuecomment-2649336174)
> This issue can be addressed with the following patch:

Yes, that's great! Thanks. Applied and added you as co-author.

💬 yancyribbens commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2649337715)
Also, the cost_of_change is computed as `563`, so any solution between target and target + cost_of_change is valid.

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2593986518)
Code review to 781c15bfca1ebaffe7b634196e19144f5ab10a50

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1949828517)
IIUC we call `ApplyRemoval` on each cluster before splitting, which calls `updated` with the graph at the end. so it seems to me that this call to `updated` before returning here is redundant and repeats work that has already been done?

I also think even if the above assumption is incorrect and an update is necessary, then since the cluster quality is now `NEEDS_RELINEARIZE`, we only need to update the locator. The second to the chunk fee rate is not necessary, because we dont return chunk fe
...

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1941918671)
In "txgraph: (feature) add initial version" c89d147209c91bb0464321f5bc733a4eeab0dea0

It is also called by the move assignment operator of `Ref`

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943139699)
In "txgraph: (feature) add initial version" c89d147209c91bb0464321f5bc733a4eeab0dea0

We can check whether `m_to_remove` is empty first so we don't have to copy the reference when it is, the copied reference is also only used in this if statement, so we can just use the `m_to_remove` value

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1949870180)
IIUC you are recommending that callers should remove transactions topologically starting from bottom descendant to the top ancestor.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1949874121)
Don't we just need to update the locator here, because all `NEEDS_RELINEARIZE` clusters has to be made acceptable before we can get their chunk feerate?

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1949967974)
Yes this is much better now!
Thanks

💬 hodlinator commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2649354598)
Succeeded with maflcko's diff applied to master (modified build dir).

Got a big diff, indicating a lot of test non-determinism:
```
[2025-02-10 22:32:56] Measuring coverage, run #1 of 2
[2025-02-10 22:35:26] Measuring coverage, run #2 of 2

The line coverage is non-deterministic between runs. Exiting.

The test suite must be deterministic in the sense that the set of lines executed at least
once must be identical between runs. This is a necessary condition for meaningful
coverage mea
...

💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2649356408)
> I wonder if it wouldn't be easier to just go with clang/llvm

I've tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack

💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2649374995)
> Could the `CAPNP_EXECUTABLE`, `CAPNPC_CXX_EXECUTABLE` and all `CapnProto_*` CMake variables be marked as advanced to avoid cluttering the CMake cache view when inspected by the user?

Agreed on the rest, but `CAPNP_EXECUTABLE` and `CAPNPC_CXX_EXECUTABLE` are relevant to cross-compilers and shouldn't be hidden. In fact, to be thorough, `multiprocess.md` should mention them as well.

🤔 ismaelsadeeq reviewed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803#pullrequestreview-2607326125)
Concept ACK

💬 ismaelsadeeq commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1949991208)
I don't think this conditional check is necessary. It will be simpler and more straightforward to just set the minimum rather than creating a new variable and then checking the conditional statement.

In the worst case, we are creating two variables here, whereas in my suggestion, we are just updating the initial variable.

```suggestion
miner_options.block_reserved_weight = MINIMUM_BLOCK_RESERVED_WEIGHT;
```