💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1678361713)
In commit "Add getCoinbaseMerklePath() to Mining interface" (1f5f16d166f8944d532aadc382b30f68a06720e2)

Instead of implementing this function in `node/interfaces.cpp` file it would be better to add a standalone `GetCoinBaseMerklePath(const CBlock&block)` function or a new `CBlock` method that the interface method could call. This would keep the `interfaces.cpp` file simple, and let it just contain glue code instead of more complicated functionality. It would also make the merkle path functiona
...

💬 achow101 commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2229460209)
c192b30156ae41638291010b40b874479ea1943c "clusterlin: add algorithms for connectedness/connected components" seems to be only relevant for the optimizations in #30286, so could be dropped from here?

👍 tdb3 approved a pull request: "test: Fix intermittent failure in p2p_v2_misbehaving.py"
(https://github.com/bitcoin/bitcoin/pull/30420#pullrequestreview-2178783087)
cr and t ACK c6d43367a1ec47c004991143f031417c4e4b8095
Repeated test in https://github.com/bitcoin/bitcoin/pull/30420#pullrequestreview-2168907582

💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2229618185)
Another update: After again seeing no data corruption for a while (since the last update above), five of the ten servers again got data corruption over a two day period. This reinforces my beliefs that (a) this data corruption depends on the contents of the blocks received by the bitcoind, and that whatever triggers it is currently relatively infrequent (about once every two weeks); and (2) the data corruption happens at some earlier time and is not detected by bitcoind until sometime later when
...

🤔 furszy reviewed a pull request: "index: Check all necessary block data is available before starting to sync"
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2179012886)
It would be nice to implement 81638f5d42b841 differently, without adding the `<chain.h>` dependency to all index headers. This dependency provides access to node internal types that indexes running on other processes (in the future) will not know about.

It seems we all implemented the same "index X requires block data" and "index Y requires block undo data" in slightly different ways. I did it in #26966 some time ago, and ryanofsky also did it in #24230.

My preference would be to follow #2
...

💬 furszy commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r1678634382)
In 81638f5d42b841f:

This include isn't needed.

💬 www222fff commented on issue "scriptPubKey no address":
(https://github.com/bitcoin/bitcoin/issues/30450#issuecomment-2229827248)
yes, only output need scriptPubKeys

👍 tdb3 approved a pull request: "kernel: De-globalize static validation variables"
(https://github.com/bitcoin/bitcoin/pull/30425#pullrequestreview-2179090811)
code review ACK 51fa26239af9bbfd44029aaf595cb4c6a8d4a75d
Nice cleanup/prep.

💬 maflcko commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2230122996)
ACK c6d43367a1ec47c004991143f031417c4e4b8095

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1678927084)
I only used `getBlockHeader()` here to test the implementation, but it's indeed not really useful in this context.

✅ fanquake closed an issue: "ci: failure in `p2p_v2_misbehaving.py`"
(https://github.com/bitcoin/bitcoin/issues/30419)

🚀 fanquake merged a pull request: "test: Fix intermittent failure in p2p_v2_misbehaving.py"
(https://github.com/bitcoin/bitcoin/pull/30420)

📝 maflcko converted_to_draft a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444)
In `rest_getutxos` outpoint indexes such as `+N` or `-N` are accepted. This should be harmless, because any index out of range should be treated as a non-existent utxo. However, a negative index can't exist ever, so it seems better to reject all signs, whether `+` or `-`.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1678947516)
It is probably fine to rework the error strings in `rest.cpp`, but seems better to leave for a follow-up, given that there are other follow-ups as well: https://github.com/bitcoin/bitcoin/pull/30444#issuecomment-2227109971 ?

This way, the changes here would be focussed on one thing (fixing the possible UB).

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678963168)
See above https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1660679371, just omitting the word "pool" because in theory there could be something _other_ than a pool that wants to add data to our coinbase, just seems confusing to me.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678967962)
#29432 implements this in more detail. I'm not even 100% sure if it's correct there, so I'd rather wait with documentation the exact numbers - that might just encourage someone to recompile and burn themselves.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678972145)
`MAX_STANDARD_TX_WEIGHT` is a standardness rule. If we wanted to enforce it in the mining code, which would be unrelated to this PR, it shouldn't be through an assert / assume.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1678979204)
No, I think the `LOCK(m_tx_download_mutex)` already checks that it isn't already held, since it's a `Mutex`.

I guess this seems a bit random, but I'm trying to establish the lock hierarchy as `m_tx_download_mutex`, then `m_mempool.cs` (which is a `RecursiveMutex`). That's also why we can only fire `ActiveTipChanged` after releasing the mempool mutex. Later code added to `TxDownloadManager` will call `CTxMemPool` functions that take the mempool lock (see e.g. https://github.com/bitcoin/bitcoin
...

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2230320294)
I took @ryanofsky's patch and split it into two commits.

⚠️ 1alexbc1 opened an issue: "Bugreport"
(https://github.com/bitcoin-core/gui/issues/829)
### Please describe the feature you'd like to see added.

Bug

### Is your feature related to a problem, if so please describe it.

Bug

### Describe the solution you'd like

_No response_

### Describe any alternatives you've considered

_No response_

### Please leave any additional context

_No response_