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

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2274401173)
> Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed

Thanks, that was not clear to me indeed.

> I haven't tried it, but https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1700350964 (the error message for the user will not be: "Corrupt metadata, block height corrupt", but instead "A forked headers-chain with more work than the chain with the snapshot base block header exists. Ple
...

📝 theStack opened a pull request: "contrib: support reading XORed blocks in linearize-data.py script"
(https://github.com/bitcoin/bitcoin/pull/30607)
This PR is a small follow-up for #28052, adding support for the block linearization script to handle XORed blocksdir *.dat files. Note that if no xor.dat file exists, the XOR pattern is set to all-zeros, in order to still support blockdirs that have been created with versions earlier than 28.x.

Partly fixes issue #30599.

💬 maflcko commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2274410714)
Dropped one commit, because a conflicting pull has already picked it up.

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2274410734)
A little recap so far: While I still don’t find the arguments *against* having the height included convincing I am currently leaning towards going with the approach in #30598 and removing it for now. My reasoning is as follows:

- While the arguments against may not be strong I conceive that it would be better if we had a clear plan for how we use the height which would give a stronger argument for keeping it in. We can re-add the height in a new version together with a clear plan of how it is
...

💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707978449)
So I was still hunting for the conceptual consensus and didn't change this yet. I think we are coming to an end though (see my comment below). It would be great if you could indicate where you stand on this though @ismaelsadeeq . If more support for the approach here I will squash the commits and implement the suggested improvements.

💬 theuni commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2274440199)
Very cool. Can't wait to dig in when I have some free time.

🤔 hodlinator reviewed a pull request: "Add a "tx output spender" index"
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-2219808525)
Concept ACK 40e44e96c38593210877f0b5d062e71a8dd292a5

Can see how this is useful for L2s. Is a variant of this PR known to be used in production?

I do agree with @Pantamis in https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1320276112 that having it take 3x the space of **TxIndex** seems unfortunate, would prefer it used `CDiskTxPos` for the values.


Commits could be squashed/rewritten. Seems like the latter two are responses to PR feedback rather than "telling a story".
Prev
...

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1704599014)
nit: Inconsistent style, should probably have initializer list on own line in both cases.

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706479714)
Suggestion: use `optional` to promote error checking or `[[nodiscard]] bool` and document non-const "out"-references by variable naming or comment.

```suggestion
std::optional<Txid> TxoSpenderIndex::FindSpender(const COutPoint& txo) const
{
uint256 tx_hash_out;
if (m_db->Read(std::pair{DB_TXOSPENDERINDEX, txo}, tx_hash_out))
return Txid::FromUint256(tx_hash_out);
return std::nullopt;
}
```