💬 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;
```

💬 ismaelsadeeq commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1949993496)
Nice moving this here, prevents creating `block_adjusted_max_weight` variable in each loop iteration.

💬 hodlinator commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949981506)
Nice touch on adding a check for non-existence of abc!
https://github.com/bitcoin/bitcoin/blob/bb8309723cdd3ce0378fbe9708bd600dfaa157aa/test/functional/feature_logging.py#L113

💬 hodlinator commented on pull request "logging: Ensure -debug=0/none behaves consistently with -nodebug":
(https://github.com/bitcoin/bitcoin/pull/31767#discussion_r1949995455)
Seems like you forgot to remove this? (`last_negated` should theoretically be able to be marked `const`, but makes line go over 80 chars).
```suggestion
```

💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#issuecomment-2649431826)
Updated this to work even after the legacy wallet is deleted.

💬 hodlinator commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2649437799)
> > Attempted to run Guix build [e717fb5](https://github.com/bitcoin/bitcoin/commit/e717fb5a429d9d00e008fa1eb2b85179050be8fe) cross-built for Windows, but it fails already on test "0" / line 133:
>
> Is this failure introduced by this PR, or does it occur on the master branch as well?

I think the *way* cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.

With the same executable (e717fb5a429d = this PR which only includes Python changes) on master
...

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2649486439)
Changed the `test each commit` CI job to use `-DWARN_INCOMPATIBLE_BDB=OFF`, otherwise it results in a fatal error if `-DWCONFIGURE_ERROR=ON`.