💬 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;
}
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706495582)
No longer used?
```suggestion
#include <common/args.h>
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706507458)
Could use `pair{}` for brevity here and elsewhere (https://stackoverflow.com/a/41521422).
```suggestion
batch.Write(std::pair{DB_TXOSPENDERINDEX, outpoint}, hash);
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706465407)
nit: Consider using `default` syntax to indicate that you only included this in the **.cpp** file so that `m_db` can be destructed:
```suggestion
TxoSpenderIndex::~TxoSpenderIndex() = default;
```
`TxIndex` already does this.

(Inspired by https://github.com/bitcoin/bitcoin/pull/19988#discussion_r494257310).

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706536603)
Suggestion: Clarify the conditional branches and state of `mempool_only` later in the code:
```suggestion
// do nothing if only mempool querying is allowed
```