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

🤔 glozow reviewed a pull request: "refactor: use recommended type hiding on multi_index types"
(https://github.com/bitcoin/bitcoin/pull/30194#pullrequestreview-2224832682)
ACK a3cb309e7c31853f272bffaa65fb6ab0a7cc4083, TIL, makes sense to me

> there should be no functional change.

Reviewed with --color-moved=dimmed_zebra, LGTM

> This significantly reduces the size of the symbol name lengths that end up in the binaries

According to `nm -C src/bitcoind | wc -c` on my machine
a3cb309e7c31853f272bffaa65fb6ab0a7cc4083: 3173329
a3cb309e7c31853f272bffaa65fb6ab0a7cc4083~1: 3227490
3227490 - 3173329 = 54161 chars, and same for `nm -C src/bitcoind | grep multi
...

🤔 BrandonOdiwuor reviewed a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598#pullrequestreview-2224858869)
Concept ACK

💬 josibake commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1706957219)
Up for grabs! Feel free to ping me for review.

💬 murchandamus commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2273420686)
> @murchandamus feel free to open the deprecation PR 👍 I'm leaving as draft as noted in OP until post-28

I am hearing that someone else is already working on that. :)

👍 maflcko approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-2224610452)
left some style-nits, feel free to ignore. lgtm

review ACK 86b38529d5014612c3e7bb59fdc4dad3bff2aa64 🥒

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvV
...

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1706891905)
style-nit: txdb.h is CCoinsViewDB (chainstate/), but you are fuzzing BlockTreeDB (blocks/index/).

Maybe just:

```
test/fuzz/block_index.cpp should add these lines:
#include <assert.h> // for assert
#include <functional> // for function
#include <memory> // for unique_ptr, make_unique
#include <optional> // for optional
#include <string> // for string
#include <utility
...

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1706914195)
q/nit: Any reason to use MAIN here, instead of the default, or a test-net?

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1706894379)
style-nit, to pre-empt touching this file on (or after) future changes.

```suggestion
// Copyright (c) 2023-present The Bitcoin Core developers
```

💬 maflcko commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1706968586)
No, it is. (At least the code I reviewed locally)