💬 fanquake commented on pull request "guix: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-2306991503)
For now, the Guix built bins could be inspected with:
```bash
# bitcoin/guix-build-30af1c56da93/output/aarch64-linux-gnu/bitcoin-30af1c56da93/bin# readelf -n * | grep "AArch64"
Properties: AArch64 feature: BTI, PAC
Properties: AArch64 feature: BTI, PAC
Properties: AArch64 feature: BTI, PAC
Properties: AArch64 feature: BTI, PAC
Properties: AArch64 feature: BTI, PAC
Properties: AArch64 feature: BTI, PAC
Properties: AArch64 feature: BTI, PAC
```

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728897817)
Added

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728898085)
Done.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728898329)
Taken

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2307003695)
> Would suggest updating the commit message to describe the change a bit better, add `bugfix` to the title, and remove the double `[]` and `:` notation:

Done, thank you.

💬 fanquake commented on pull request "guix: GCC 12 consolidation":
(https://github.com/bitcoin/bitcoin/pull/30511#discussion_r1728900761)
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=9aa38f98cbd41644abe2c43bb4f44f74df642df2 - landed, so we can follow up here.

🤔 stickies-v reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2257111871)
Force-pushed to address all review comments, limited to nits and tests so should be an easy re-review:
- [grammar nit](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726880388) for `RANDOM_CTX_SEED`
- [added](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727507122) `from_user_hex` test case to cover more > 64 char cases and cleaned up the test a bit by replacing `valid_hex_65` with `valid_hex_64` which should be less confusing
- [re-added mixed case testing](https://g
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728922692)
yeah, I could've sworn a previous version introduced this newline and this was me reverting it, but I suppose it must be a rebase artifact, thanks for spotting and fixed now.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728905343)
Resolved by [replacing](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727507122) `valid_hex_65` with `valid_hex_64` which I think addresses the confusion, too?

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728920638)
Hmm, fair point. I think I dropped it because of #30560 dropping mixed case support for the consteval ctor, but that's unrelated to this pull of course so keeping the test is the right approach. I've added mixed case into 4 existing checks, but I'd still like to drop the `Ffa` tests since we can't use the `uint8_t` ctor which would make an equality test quite verbose for I think nothing that's not already tested in the other lines?

```cpp
BOOST_CHECK_EQUAL(uint256::FromUserHex("0xFf").va
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728903797)
Done, and reorganized the test a bit to use `std::string valid_hex_64` instead of `std::string_view valid_hex_65` which makes cat operations significantly less verbose, and makes the test more readable imo.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1728890062)
Thanks, taken.

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2307049315)
re-ACK 245dfa20bd37f78783adbd03897b7f144d71a7b8

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2307070733)
re-ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156

`git range-diff master 81554aac80bf2270db977c110c37acc7e8034194 18d65d27726bf9fc7629b8e794047a10c9cf6156`

Changes: Grammar, added&adjusted tests (passed), undid unintentional new line.

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307077927)
```
$ FUZZ=txdownload_impl src/test/fuzz/fuzz solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt
test/fuzz/txdownloadman.cpp:384 operator(): Assertion `!txdownload_impl.RecentRejectsFilter().contains(package.back()->GetWitnessHash().ToUint256())' failed.
```

[solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt](https://github.com/user-attachments/files/16729173/solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt)

💬 hodlinator commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307081520)
Thinking about it some more, it makes sense that if we give all jobs the same random seed and 97e16f57042cab07e5e73f6bed19feec2006e4f7 makes the test folder generation deterministic, the jobs will trample over each other. If that is the case, it's probably better bring back some non-determinism in the folder generation.

💬 maflcko commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2307116476)
Correct. The easiest fix would be to just use `GetStrongRandBytes`, however, I am not sure how slow that'd be in the worst case.

Another possible fix would be to move the SeedRandomForTest after the fuzz target's init function, but before ResetCoverageCounters. However that'd still break if the fuzz target spun up a TestingSetup.

Another possible fix would be to instead use the hash of the current folder generator, as well as the hash of the pid. Which may obviously break once the kernel w
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307124171)
> [solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt](https://github.com/user-attachments/files/16729173/solution-60b4aafd2c355198b269114d31545a12b9059fd6.txt)

`Find1P1CPackage` doesn't actually check the normal reject filter at all with respect to the child, only reconsiderable, so that assertion gets hit. I suspect that's something we don't want it to do, even if it's not possible to hit inside net_processing.

LGTM modulo existing concerns about the `tx_download_one_honest_peer` harn
...

🤔 glozow reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2256916267)
ACK 12760a57b3a

Agree with keeping "reduce computation of unnecessary pot sets", intuitively it seems useful in the average situation and quite cheap.

Thanks for dropping "avoid jump ahead in unmodified pot sets," I wasn't convinced that processing the existing pot txns in the exclusion branch is always redundant or faster to skip. It also made the code harder for future people to understand, which I think is important.

I redid code review, all pretty straightforward now.

Prettified
...

💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1728757747)
781e0fb3aa5: maybe paranoid, but
```suggestion
transactions.Set(pos);
if (Assume(!transactions[pos])) feerate += depgraph.FeeRate(pos);
```