💬 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
...

📝 JoKacar opened a pull request: "Revert "multiprocess: Add basic type conversion hooks""
(https://github.com/bitcoin/bitcoin/pull/29463)
Reverts bitcoin/bitcoin#28921

✅ achow101 closed a pull request: "Revert "multiprocess: Add basic type conversion hooks""
(https://github.com/bitcoin/bitcoin/pull/29463)

💬 ryanofsky commented on pull request "refactor: Use util::Result class for wallet loading":
(https://github.com/bitcoin/bitcoin/pull/25722#issuecomment-1958356339)
Rebased 1b2a5f12b42543d13a4bcafb9585e3d1df488914 -> 6700299bcb6f560e9e4caf34254d7d86d4e78833 ([`pr/bresult-load.19`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.19) -> [`pr/bresult-load.20`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult-load.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult-load.19-rebase..pr/bresult-load.20)) on top of #26022 using #26022 to simplify some changes in this PR

💬 andrewtoth commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1498436377)
I think what @naumenkogs is referring to here is that if this condition is true then `pindex` will hit this condition on the first `pindex` in the loop and every `pindex` up until this condition is no longer true. This can be more efficient by just bumping `pindex` to the first `pindex` that is past the network limited blocks threshold.

So, we can either change the loop from range based to regular for-loop and advance the index, or move this check out of this loop and into the loop on line 14
...

💬 andrewtoth commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1498411855)
nit: `const bool is_limited_peer{IsLimitedPeer};`