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

👍 hodlinator approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2251909699)
ACK cf88a55e97719aabd62f0b608df0800fef8304de

I really like the `TrySanitizeHexNumber` -> `FromUserHex` change!
Sorry for not understanding that you had it in the works with my prior suggestion.
Only ignorable nits left.

(Still fails at the end of `make check` due to unlucky base commit).

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725750590)
(Thread https://github.com/bitcoin/bitcoin/pull/30569/files#r1725694129)
(This has been silently nagging me as well, but totally understand if you want to leave it for someone to follow up on).

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725743234)
nit: Intentionally leaving out 0x-variant?

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725778828)
> There is yet another bug

Fixed

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1725799275)
nit: I like how you responded to https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1704599014 but my primary intention was actually doing:
```suggestion
TxoSpenderIndex::DB::DB(size_t n_cache_size, bool f_memory, bool f_wipe)
: BaseIndex::DB(gArgs.GetDataDirNet() / "indexes" / "txospenderindex", n_cache_size, f_memory, f_wipe)
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1725805158)
nit: Should also use either snake_case `tx_id` or preferably `txid` to be consistent with the `Txid` type.

Could rely on `.value()` [throwing exception](https://en.cppreference.com/w/cpp/utility/optional/value) if not set and avoid the naming altogether.

\+ Slightly better for error messages etc to use `BOOST_CHECK_EQUAL`.

```suggestion
BOOST_CHECK_EQUAL(txospenderindex.FindSpender(spent[i]).value(), spender[i].GetHash());
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1725795513)
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c calls for snake_case and `Txid` is the name of the type in this codebase so `spending_txid` would fit better.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1725864956)
Taken, thanks!

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1725865325)
Done