💬 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

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1498399126)
Done

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1498402745)
I think this check is specifically done to see if the block is pruned, hence the error message `(pruned data)`. This is taken verbatim from the above function, `GetBlockChecked`, which all it does inside the locked block is check if it is pruned or not. Here we also extract the `FlatFilePos`. However, if `blockindex.nStatus & BLOCK_HAVE_DATA` is `false` then the call to `ReadRawBlockFromDisk` will fail and throw the message `Block not available`. The first commit addresses the undefined behavior
...

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1498403063)
Done, added a commit description addressing this.

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1498403179)
Done, added a commit description addressing this.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1958232928)
Updated 4ec6b060a80045049adc53b4db0b0837ba169cfc -> 20556598140030237861d21a61f646252002ddff ([`pr/bresult2.45`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.45) -> [`pr/bresult2.46`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.46), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.45..pr/bresult2.46)) making a few changes to improve compatibility with #26022

💬 ryanofsky commented on pull request "Add util::ResultPtr class":
(https://github.com/bitcoin/bitcoin/pull/26022#issuecomment-1958326867)
Rebased a1dddc5a2a86908d9ea677875ad67a79d2c71d14 -> 8ecac0885ef5b6fc2313cd2f8dfb48c10a05db27 ([`pr/bresult-ptr.3`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-ptr.3) -> [`pr/bresult-ptr.4`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-ptr.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-ptr.3-rebase..pr/bresult-ptr.4)) on top of https://github.com/bitcoin/bitcoin/pull/25665.

I also rewrote the commit and PR description. I think this is in a good sta
...