💬 ryanofsky commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725702791)
re: https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725675521

That all sounds good but I think I'm proposing a smaller not bigger change in behavior by just fixing the bug instead of trying to do something grander. The change based on master is:

```diff
--- a/src/test/prevector_tests.cpp
+++ b/src/test/prevector_tests.cpp
@@ -209,9 +209,8 @@ public:
}

prevector_tester() {
- SeedRandomForTest();
- rand_seed = InsecureRand256();
- rand_cac
...

💬 sipa commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725703682)
Can you elaborate on how you think we should in general decide where configuration should live (default in configure/cmake vs depends vs guix)?

My thinking would be that depends builds are intended to be as close to production binaries as we can get them, but without having a deterministic build environment (so without controlling compiler or dependency versions).

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725690444)
Nit (feel free to disregard): Based on the description (`Minimum work assumed to exist on a valid chain in hex`), "chain" and "value" might be superfluous here
```suggestion
return util::Error{strprintf(Untranslated("Invalid minimum work specified (%s), must be up to %d characters hex"), *value, uint256::size() * 2)};
```

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725693175)
Since this is a "User" centric specialization of `FromHex` (and not a `Hex` specialization of `FromUser`), I'd keep the `FromHex` prefix, i.e.: `FromHexUser` or perhaps `FromHexLenient`, since the `User` part is just incidental

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725694129)
Could we extract this this as a constant and use it throughout this PR (e.g. in error messages and tests)?

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725701490)
```suggestion
BOOST_CHECK_EQUAL(uint256::FromUserHex("0x10").value(), uint256{0x10});
BOOST_CHECK_EQUAL(uint256::FromUserHex("10").value(), uint256{0x10});
BOOST_CHECK_EQUAL(uint256::FromUserHex("0xff").value(), uint256{0xff});
BOOST_CHECK_EQUAL(uint256::FromUserHex("ff").value(), uint256{0xff});
```

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725695133)
Are we sure this behavior should be written in stone? It rather seems like an error to me

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725699517)
could we move this closer to first usage?

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725705844)
nit: I was wondering if it accepts `00` as well (since numbers usually trim leading zeros) and if it accepts `1` (since other implementations require an even number of digits), consider extending the examples:
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex("00").value(), uint256::ZERO);
BOOST_CHECK_EQUAL(uint256::FromUserHex("1").value(), uint256::ONE);
```

💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725715000)
> without controlling ... dependency versions

I might be misunderstanding you, but one of the main reasons depends exists is to control dependency versions? i.e as-opposed to using whatever ships with the system packages.

> so without controlling compiler

This change is a good example of trying to add that control; i.e hardcoding compiler flags with no check for if they are actually supported/or work as expected.

> Can you elaborate on how you think we should in general decide where
...

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2302954316)
namespace + formatting change
ACK 7a4d249267cb5f63ace96a5fcc03452acc5468b5

⚠️ cryptoquick opened an issue: "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs"
(https://github.com/bitcoin/bitcoin/issues/30692)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I've been struggling with the errors mentioned here for a little while now:
https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2301642234

I'm running this on a desktop machine, not in the cloud, and on external SATA ext4 disks because my main system drive is formatted with Btrfs, which doesn't work very well with key-value databases, I've found.

### Expected behaviour

I get
...

💬 sipa commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725719622)
> I might be misunderstanding you, but one of the main reasons depends exists is to control dependency versions? i.e as-opposed to using whatever ships with the system packages.

Err, yes, of course. I had compiler-infused dependencies like glibc in mind, but that's the exception rather than the rule.

> This change is a good example of trying to add that control; i.e hardcoding compiler flags with no check for if they are actually supported/or work as expected.

Ah that makes sense. Your
...

👍 hebasto approved a pull request: "Fix maybe-uninitialized warning in IsSpentKey"
(https://github.com/bitcoin/bitcoin/pull/30691#pullrequestreview-2251896951)
ACK 17707db939cb09a16c002d226152e71fce9289f2, tested on Ubuntu 24.04. The warning has been silenced for both GCC 13.2.0 and 14.0.1.

However, it still seems false positive to me.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725758066)
Yeah, your diff looks good and I understood it the first time. However I think `SeedRandomForTest` should remain `void` for now, because it wasn't intended to put the burden of logging or storing the seed onto tests. It seems harmless at first glance, but it doesn't help the dev much, because they'd still have to look up how to set the seed to reproduce the failure. Simply relying on the log (which states the name of the env var, as well as the value of the env var) should be enough.

There is
...

💬 sipa commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2303027263)
This looks like a LevelDB corruption inside the txindex index. Does `-reindex` wipe those? If not, that would explain why a reindex doesn't fix the situation.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725741207)
nit: Noticed you changed this back from the modern `LogInfo` in 855784d3a0026414159acc42fceeb271f8a28133 to the legacy `LogPrintf`, was that intentional?

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725742566)
(Thread https://github.com/bitcoin/bitcoin/pull/30569/files#r1725695133)
Good to document current behavior, whatever it is.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725732478)
nit: "characters hex" -> "hex digits" here and below - better order grammatically, more precise, and shorter.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725748797)
(Thread https://github.com/bitcoin/bitcoin/pull/30569/files#r1725693175)
I prefer the current `uint256::FromUserHex` over `uint256::FromHexUser` (the latter makes me think of sorcerers). (`uint256::FromHexLenient` is okay to).