💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1498025796)
can be constified same than `OUTBOUND_FANOUT_DESTINATIONS` and `INBOUND_FANOUT_DESTINATIONS_FRACTION` as explanation for this value is tight to them

💬 sdaftuar commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1498032703)
Do we care about the case where the other child is not itself v3 and is not signaling for opt-in-rbf? As this can only happen as the result of a reorg, I don't think we care, but maybe it's worth mentioning somewhere that we'd process the replacement anyway.

💬 achow101 commented on pull request "test: Fix SegwitV0SignatureMsg nLockTime signedness":
(https://github.com/bitcoin/bitcoin/pull/29400#issuecomment-1957562604)
ACK fab15723b0518acbb1015e64df47dcac0187e92f

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1957566802)
> At the risk of having asked this previously: Why not postpone the [1c7d8be](https://github.com/bitcoin/bitcoin/commit/1c7d8bea4f0b25f9adb89e402a130fa114220494) commit to a later pull? This would also make review easier, because the code change comes with the changes to actually use it in real code, outside of just unit tests.

Sure, I'm happy to split up this PR.

I don't think this idea came up before (or I can't remember if it did). From my perspective commit 1c7d8bea4f0b25f9adb89e402a13
...

🚀 achow101 merged a pull request: "test: Fix SegwitV0SignatureMsg nLockTime signedness"
(https://github.com/bitcoin/bitcoin/pull/29400)

💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1498086476)
I agree that we shouldn't care about it signaling v3 or BIP125. Will add comment.

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1498102589)
```suggestion
// the descendant count is done separately in SingleV3Checks for v3 transactions.
```

🤔 cbergqvist reviewed a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1893059876)
Generally great debugging UX improvement in my eyes!

💬 cbergqvist commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1497615599)
Why not pass `lock_dir` here into `fs::remove_all()` instead of the datadir-subdirectory `m_path_root`? Seems like current version does NOT remove old test directories, just the datadir.

This contradicts the PR summary:
> When the test starts, remove <datadir>/test_temp/<test-name> if it exists from an earlier run (currently, it's presumed not to exist due to the long random string)

Current (7b81dea) behavior:
```sh
$ ./src/test/test_bitcoin --run_test=getarg_tests/doubledash -- --testd
...

💬 cbergqvist commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1497443377)
Also reacted to this, but another bonus of having the intermediate `test_temp` dir is that one doesn't unintentionally flood an already existing directory with directories based on test-names.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1957738284)
Rebased f822ac9a24d684937f1258da89812e99c4b205ba -> 4ec6b060a80045049adc53b4db0b0837ba169cfc ([`pr/bresult2.44`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.44) -> [`pr/bresult2.45`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.45), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.44-rebase..pr/bresult2.45)) due to silent conflict with #27877

💬 cbergqvist commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1498243817)
> What is the purpose of this extra levels? This can just be `root_dir / G_TEST_GET_FULL_NAME()`.

Also reacted to this, but another bonus of having the intermediate `test_temp` dir is that one doesn't unintentionally flood an already existing directory with directories based on test-names.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1498264430)
I was unable to reproduce the error that @maflcko reported.

Analyzing the line, I don’t see how the content of the parentheses could ever become negative. At most, it would become temporary negative, when `max_spendable` and `max_output_groups` have already been deducted, but `group_pos.size()` has not yet been added.

Would it perhaps help if the addition were put to the front so it never even temporarily goes negative?

```diff
-const CAmount eff_value{fuzzed_data_provider.ConsumeInteg
...

📝 murchandamus opened a pull request: "[fuzz] Avoid partial negative result"
(https://github.com/bitcoin/bitcoin/pull/29462)
May address the problem reported by @maflcko in https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1890304914.

For some values, `MAX_MONEY - max_spendable - max_output_groups` could result in a partial negative value. By putting the addition of `group_pos.size()` first, all partial results in this line will be strictly positive.

I opened this as a draft, since I was unable to reproduce the issue, so I’m waiting for confirmation whether this in fact mitigates the problem.

💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1498323656)
> I was unable to reproduce the error that @maflcko reported.

I was able to reproduce if I enabled the integer sanitizer.

> Would it perhaps help if the addition were put to the front so it never even temporarily goes negative?
>
> ```diff
> -const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
> +const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY + gro
...

👋 murchandamus's pull request is ready for review: "[fuzz] Avoid partial negative result"
(https://github.com/bitcoin/bitcoin/pull/29462)

💬 murchandamus commented on pull request "[fuzz] Avoid partial negative result":
(https://github.com/bitcoin/bitcoin/pull/29462#issuecomment-1958010837)
Opening for review:
After @achow101 pointed out that I needed to enable the "integer" fuzz sanitizer, I was able to reproduce the issue and verify that the proposed fix mitigates the problem.

💬 murchandamus commented on pull request "[fuzz] Avoid partial negative result":
(https://github.com/bitcoin/bitcoin/pull/29462#issuecomment-1958011159)
Opening for review:
After @achow101 pointed out that I needed to enable the "integer" fuzz sanitizer, I was able to reproduce the issue and verify that the proposed fix mitigates the problem.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1498351205)
Thanks, I guess I misinterpreted `SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change ../../src/wallet/test/fuzz/coinselection.cpp:161:89 in` as needing the "undefined" sanitizer, but didn’t realize that there was one for "integer". I was able to reproduce and verify that changing the order of operations mitigates the issue here.

💬 sdaftuar commented on issue "Gathering Priorities of 28.0":
(https://github.com/bitcoin/bitcoin/issues/29439#issuecomment-1958088049)
Cluster mempool