💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706853629)
> > Can you run this in valgrind?
>
> Thanks for the context @maflcko! Valgrind currently isn't really [compatible with ARM-based Macs](https://github.com/LouisBrunner/valgrind-macos/issues/56).

An alternative on macos may be Asan (or possibly Ubsan), but they require re-compiling the binary.

💬 maflcko commented on pull request "test: update satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2273276372)
review ACK 7d7671ad5c83c3d1220d84110259c63b5ed7405d

The pull request description will need to be updated to reflect the current state.

💬 theStack commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2273289853)
As expressed in https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273127594, I have a slight preference for keeping the block height for informative purposes (showing a block height is just more user friendly than showing block hash, for external tooling) and just verifying it as well (see e.g. https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273230354), but happy to review this PR if there is consensus that we don't need it.

💬 maflcko commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273293278)
I haven't tried to compile that suggestion, but it'll probably fail `-Wsign-compare`. Also, the problem remains that future code may accidentally use it before it is checked (or after the check failed). So I'd say that a rename to `m_base_blockheight_untrusted` could still make sense. An alternative may be to move the metadata parsing into the `ActivateSnapshot` function itself (instead of parsing it outside and passing it to the function, exposing it to a greater scope than needed). Any metadat
...

💬 glozow commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2273300245)
Are you still working on this?

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1706894403)
In 86b38529d5014612c3e7bb59fdc4dad3bff2aa64: `nBits` is not hardcoded anymore.
```suggestion
// Hardcoded block hash to make sure the blocks we store pass the pow check.
```

💬 Sjors commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2273333251)
> I have a slight preference for keeping the block height for informative purposes

Ditto. If the user tries to load a snapshot during header sync, we can give an indication of how long they have to wait. If we're already past the expected height, we can say something useful about potential forks.

I'll read up on the discussion.

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706867937)
Do I understand it correctly that the point of this kind of testing (i.e. just calling the method with random values without validating the result) is to make sure we don't have memory problems, don't throw unexpected exceptions, etc?

Since we seem to have many hex related methods, would it make sense to compare their outputs, to make sure we at least have internal consistency?
```suggestion
auto sanitized_hex = TrySanitizeHexNumber(random_hex_string, result_size);
if (sanitized_he
...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706877603)
is it deliberate that inputs don't always contain an even number of digits?

🤔 paplorinc reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2224497224)
Thanks, please see my remaining suggestions

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706875949)
I might have mentioned this before, but what's the reason for empbracing mixed-case hexadecimal values?

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706891172)
in the next commit we're using `uint256::size() * 2` instead of the hard-coded 64 - consider unifying, if you agree that it documents the behavior slightly better

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706884207)
That might not work, but extracting might:
```C++
auto final_size = (result_size < 0) ? input.size() : static_cast<size_t>(result_size);
```

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706878513)
could we use `IsHex` here instead, i.e. merge it with previous line:
```C++
if (input.empty() || ((int)input.size() > result_size) || !IsHex(input)) return std::nullopt;
```

though this contains an extra evennes requirement which we may not want here.

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706897088)
nit: if you think it's more readable, here's an alternative which avoids reparsing and duplicating `uint256::size() * 2` (but assigns twice, so you may not like it):
```C++
if (auto value{args.GetArg("-assumevalid")}) {
if (*value == "0") { // handle -noassumevalid
opts.assumed_valid_block = uint256{};
} else if (auto block_hash{uint256::FromHex(*value)}) {
opts.assumed_valid_block = *block_hash;
} else {
return util::Error{
...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706902315)
I've noted it and will give it a try next time, thank you

💬 Sjors commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2273342425)
As mentioned in https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2273333251 I also prefer to keep height around and just check it.

Maybe make it a `uint64` if you have to touch it anyway.

💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1706914584)
Are you working on this or should I do it?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2273368482)
I come bearing good news again \\:D/
```python
Benchmark 1: COMMIT=df34a9 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0
Time (mean ± σ): 9216.193 s ± 165.608 s [User: 10809.113 s, System: 1931.597 s]
Range (min … max): 9065.956 s … 9393.770 s 3 runs

Benchmark 2: COMMIT=589db87 ./src/bitcoind -datadir=/mnt/sda1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=500000 -printtoconsole=0
Time (mean ± σ):
...

💬 Sjors commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2273376255)
There's two things I'd like to be able to do in the future. One doesn't require having the height in the metadata, but the other does.

1. I'd like to keep the option to improve the `The base block header (%s) must appear in the headers chain.` message by checking the snapshot height.

In the normal case, where header sync is still in progress, we can change it to:

```
The base block header (%s) must appear in the headers chain at height %d (currently synced to %d).
```

If we already
...