🤔 mzumsande reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2226187388)
re-ACK 589db872e116779ab9cae693171ac8a8c02d9923

💬 furszy commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet` - (`void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion 'it.second' failed.`)":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274343867)
@ryanofsky, I'm not seeing how multiple threads can reach `UnloadWallet` at the same time for the same wallet. `unloadwallet` first calls `RemoveWallet()` then `UnloadWallet()`. And `RemoveWallet()` locks the wallets context mutex and removes the wallet from the context. So the next thread executing `RemoveWallet()` would just return early there (the RPC would return an error) and never reach the follow-up `UnloadWallet` call.

💬 maflcko commented on issue "fuzz: crypto_fschacha20poly1305 timeout":
(https://github.com/bitcoin/bitcoin/issues/30505#issuecomment-2274344203)
> Found the timeout really fast by running:

The fuzz input should also be in the attachment in the pull request description, which also allows to reproduce it.

💬 maflcko commented on pull request "test: [refactor] Use g_rand_ctx directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2274352643)
> But it could also help down the road, for example if we wanted to remove the global, or run test cases deterministically in parallel.

I looked into it. However, it seems more work. My findings:

* There are ~300 lines of code touched, which can use the member alias or the global
* 108 lines can not use the member alias as-is, because the compiler does not see it in their scope, so they'll need to be fixed up manually one-by-one
* Excluding them (and only them) automatically leaves some
...

🤔 hodlinator requested changes to a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2225982967)
Concept ACK d045fc7ac1729cf29140c43518f20375f2aaa1cc

Thanks for chipping away at making hex string handling more robust!

Some minor disagreements + ignorable nits. The most significant disagreement in this thread: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1700718302

Tested:
```bash
$ RANDOM_CTX_SEED=1231231231231231231231231231231231231231231231231231231231312312 src/test/test_bitcoin
$ test/functional/feature_assumevalid.py
```

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707728879)
ni: If padding were on a warm code path I would advocate for reserving the final size of the `result` string first, avoiding the temporary '0'-repeated string and appending `input` after the zeroes.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707734248)
Why document the type here when it appears in the code below?
Seems noisy if we were to do that for return types everywhere.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707760135)
`constexpr` implies `inline` for functions, doesn't it also for variables? Never used `inline` on variables myself, but seems it is useful in **.h** files to avoid ODR-issues. This is a **.cpp** file though.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707914379)
nit:
```suggestion
BOOST_REQUIRE(args_man.ParseParameters(argv.size(), argv.data(), error));
BOOST_REQUIRE(error.empty());
```

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707739633)
nit: Would personally merge the next commit (e6f81b9d81359a7ffeff6d1830188f00df0e8db0) into this one (a227cb511ec948b37ddbc9ee65de586109ebc1da) since you're already touching this line and it looks weird as an atomic commit to just throw away the sanitized value.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707706392)
> And we can't use `BOOST_CHECK_EQUAL(set_opts({"-assumevalid=0"}).assumed_valid_block, std::make_optional(uint256::ZERO))` since there's no standard way to display optionals, right?

Would this fit your definition of displaying optionals?

https://github.com/bitcoin/bitcoin/pull/16545/files#diff-d4a2fb26adedc27f16bd3778424fa94c473342a695b228220a1810119028be5bR48-R64

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707787408)
To me asserts are for enforcing logical consistency of the code, NOT for handling invalid values of environment variables.

A slight convention seems to be using `abort()` in these circumstances.
```C++
if (auto num_parsed{uint256::FromHex(num)}) {
return *num_parsed;
} else {
std::cerr << RANDOM_CTX_SEED << " must be a " << uint256::size() * 2 << " char hex string without '0x'-prefix, was set to: '" << num << "'." << std::endl;

...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707936724)
Thanks, I think that would produce better error messages on failure.

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707941944)
> To me asserts are for enforcing logical consistency of the code, NOT for handling invalid values of environment variables.

Correct. This rule must be applied to real production code. However, in the tests, personally I like to use it for brevity. No objection to using something else.

📝 aguycalled opened a pull request: "Create wallet from seed"
(https://github.com/bitcoin/bitcoin/pull/30606)
This PR adds the option to create a wallet from a seed. The seed of a wallet can be obtained using the RPC command `getblsctseed`.

Two options to create a wallet:
- Using the `navio-wallet` tool together with the option `seed`. Example:
`navio-wallet -blsct -chain=blsctregtest -wallet=wallet_name -seed="71187dc33c03914c630e87b205777d51f7bd2749bc1164795e0134544d43d9ee" create`
- Using the rpc command `createwallet`. Example:
`navio-cli --blsctregtest -named createwallet wallet_name=walle
...

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707958169)
See the description: "The first commit shows how to reproduce the error shown in https://github.com/bitcoin/bitcoin/issues/30514 and how it could be fixed with a minimal change. The second commit adds an explicit sanitizer that will probably be the preferred approach. I can squash the commits but first I would like to hear conceptual feedback since https://github.com/bitcoin/bitcoin/issues/30514 suggested to remove the block height instead."

👍 maflcko approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2226246193)
left a nit. Feel free to ignore.

Otherwise this looks good on a first glance.

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707958805)
If you re-touch this commit, it could make sense to remove the `__func__` printing in the log call below. It doesn't add any value and may be duplicate. I understand it is unrelated, but when touching a test-only function, may as well fix all trivial issues in one go. (See the commit I referred to in https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1703942199, which also has steps to reproduce and test in the description)

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707962074)
We would probably have to cast somewhere else then because `nHeight` is `int`.

✅ maflcko closed a pull request: "Create wallet from seed"
(https://github.com/bitcoin/bitcoin/pull/30606)