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

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706511696)
Superfluous comment copied from **txindex.h**.
```suggestion
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706514167)
nit: Explain why, not what:
```suggestion
// Destroys unique_ptr to an incomplete type.
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706531361)
nit: I realize this is copied from **txindex.h**, but could be made less redundant:
```suggestion
/// May be null.
extern std::unique_ptr<TxoSpenderIndex> g_txospenderindex;
```

✅ furszy closed a pull request: "assumeUTXO: snapshot load, update VERSION msg height"
(https://github.com/bitcoin/bitcoin/pull/30418)

💬 furszy commented on pull request "assumeUTXO: snapshot load, update VERSION msg height":
(https://github.com/bitcoin/bitcoin/pull/30418#issuecomment-2274486818)
Thanks for the review [fjahr](https://github.com/fjahr)! Sorry for the delay. And thanks for the ping maflcko.

> We send this usually when we have a new block but the loading of the snapshot does not necessarily mean that we have the block, right? We only need the header for that. So we should probably set it when we get block itself I think.

Yeah, we set this field when a new tip block is connected and only send it during the peer connection's initial handshake. I initially found it odd b
...

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1708102729)
Indeed, done.

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1708102944)
Indeed, fixed.