💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2229124568)
@pinheadmz @vasild

Ready for you to take a second look if you have any time.

👍 hebasto approved a pull request: "gui: rendering an amp characters in the wallet name for QMenu"
(https://github.com/bitcoin-core/gui/pull/828#pullrequestreview-2178389009)
re-ACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0.

CI failures are unrelated.

💬 hebasto commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2229220553)
Another one: https://cirrus-ci.com/task/5588622949220352

📝 furszy converted_to_draft a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807)
Bundle all function's outputs inside the `util::Result` returned object.

Removals:
- The input-output 'change_pos' ref arg from `createTransaction`, which has been a source of bugs in the past.
- The 'fee' ref arg from `createTransaction`, which is currently only set when the transaction creation process succeeds.
- The currently unreachable `AmountWithFeeExceedsBalance` error (more info about its re-introduction at https://github.com/bitcoin/bitcoin/pull/25269).

Additionally, this PR m
...

💬 furszy commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2229224539)
Drafted until https://github.com/bitcoin/bitcoin/pull/25269 gets merged. This is a continuation of that work.

💬 TheBlueMatt commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2229233817)
> This PR returns a BlockTemplate interface which causes the node to hold on to the template and its block.
> The goal is to be able to quickly send NewTemplate and after that to copy all transactions to the external application so it can respond to RequestTransactionData and SubmitSolution.
> I assume your concern is that this would make SubmitSolution too slow because the external application needs to pass the whole block back via IPC?

Ah, okay, that was my only real concern. Sending tran
...

👍 pinheadmz approved a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2178514340)
ACK 23333b7ed243071c9b4e4f04c727556d8065acbb

Reviewed updated code change and tested in linux VM with local ipv6 only.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 23333b7ed243071c9b4e4f04c727556d8065acbb
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaVfcgACgkQ5+KYS2KJ
yTorBw/7BFoooqdMN+Zb000m/qKsUoMhW36vZBdGTuxt3xkWrRHlggm8ym/fQfen
jt9VcWYl6vmZFnU+nUvJbdN+GLTdf+fDT8vdJdYKjKSxmflEIGLRnf5Pktf2E
...

💬 pinheadmz commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1678310386)
If you need to retouch, you might consider using a local `addrinfo` variable for the second call since it serves a specific purpose only inside this if condition. It doesn't matter because it's not like it gets used again later, just something I noticed.

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2229281988)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/263.

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1678338506)
Don't have strong feeling either way but there is an argument that doing it with one variable means any changes to flags passed to the first call get automatically added to the second call which is I think better default behaviour.

🤔 mzumsande reviewed a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2178557577)
Concept ACK - don't know the txrequest logic very well though.

The PR description should be updated (`UpdatedBlockTipSync` was renamed).

💬 mzumsande commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1678336506)
I think you meant `m_tx_download_mutex`

👍 ryanofsky approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2178579385)
Light code review ACK 408c11e9bced08c51a7645a2de2c39c18e4360d9. This looks good, but I didn't review the getCoinbaseMerklePath implementation closely to check that it is computing the right thing.

💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1678349206)
In commit "Have createNewBlock return BlockTemplate interface" (4af3d9a483e4107301f0f0a42da781dfa79acb82)

Should be possible to avoid getBlockHeader calls by just using the `block` variable:

```diff
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -941,7 +941,7 @@ static RPCHelpMan getblocktemplate()
result.pushKV("vbavailable", std::move(vbavailable));
result.pushKV("vbrequired", int(0));

- result.pushKV("previousblockhash", block_template->getBlockHeader().hashP
...

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