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

👍 TheCharlatan approved a pull request: "refactor: use recommended type hiding on multi_index types"
(https://github.com/bitcoin/bitcoin/pull/30194#pullrequestreview-2224980727)
ACK a3cb309e7c31853f272bffaa65fb6ab0a7cc4083

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706982102)
I was thinking of business to business companies using Testnet3 as a development environment. They may have dozens of enterprise clients that have built Testing infrastructure on Testnet3 and serve transaction data by interfacing with Bitcoin Core. Even if the service provider is willing to upgrade, it can be a pain to get the customers to update (speaking from experience). I’m fine with deprecating Testnet3, but I would suggest that the option should only be removed from Bitcoin Core with v29.
...

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-2273455486)
Is there some label or something that can assist people in discovering this kind of follow-up work on closed/merged PRs?