💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728873933)
```suggestion
// concurrently, ensuring no race conditions occur during either process.
```

💬 stickies-v commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1728871804)
nit: while touching adjacent line, could help a neighbour out
```suggestion
#include <cstdint>
#include <future>
#include <memory>
#include <thread>
```

💬 fanquake commented on pull request "guix: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-2306986439)
Rebased on master and dropped a commit, also bumped the glibc 2.33 branch to the latest commit. Still based on #30433, but the main blocker here remains the glibc bump.

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728887570)
To avoid the circular dependency, you could get rid of this file and move its contents to `txdownloadman_impl.cpp`.

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