🤔 stickies-v reviewed a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556786030)
Concept ACK, did a first run-through of the code but will dig into it deeper.

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280862595)
nit: `const DataStream& ssKey`?

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280723077)
`WriteImpl`, `ssKey` and `ssValue` are all `CDBBatch` class members, I'm not sure passing these as parameters makes sense? And if so, I think `ssKey` should be `const`.

At first sight, I'm not entirely sure why `ssKey` and `ssValue` are `CDBBatch` members when they get resized and cleared in every operation, so perhaps cleaning that up is the better way to go, but I don't like the ambiguity the current implementation introduces.

And this comment applies pretty much identically to 5c9e87fcd
...

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280498519)
nit: `const std::string&`?

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280943436)
nit: `const DataStream& ssKey`?

💬 ryanofsky commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280950527)
In commit "wallet: return and display signer error" (0e0163a062b09a3f762d7dd0af3d6e78e675aae1)

s/translated/original/ here since RPC methods return untranslated error strings

📝 ryanofsky converted_to_draft a pull request: "refactor: Use util::Result class for wallet loading"
(https://github.com/bitcoin/bitcoin/pull/25722)
**This is based on #25665.** The non-base commits are:

- [`17891a95ab58` refactor: Use util::Result class for wallet loading](https://github.com/bitcoin/bitcoin/pull/25722/commits/17891a95ab5873aa978a5bf5ed8985ee1513e728)

---

Wallet loading functions up and down the stack have lots of error and warning parameters, and return error information in different ways. This PR makes them uniformly return `util::Result`, without changing behavior.

📝 sipa opened a pull request: "BIP324 transport support"
(https://github.com/bitcoin/bitcoin/pull/28196)
Builds on top of #28008 and #28165, and part of #27634. Draft while dependencies and (more) tests are missing.

This implements BIP324 v2 transport layer. It is currently only accessible through the test-only `-bip324=` command-line option, which specifies IPs, host names, or subnets for which to use BIP324 connections. This option is only added in order to make experimentation possible; I don't expect it will be supported long-term (or should even remain in this PR).

Still missing features
...

📝 sipa converted_to_draft a pull request: "BIP324 transport support"
(https://github.com/bitcoin/bitcoin/pull/28196)
Builds on top of #28008 and #28165, and part of #27634. Draft while dependencies and (more) tests are missing.

This implements BIP324 v2 transport layer. It is currently only accessible through the test-only `-bip324=` command-line option, which specifies IPs, host names, or subnets for which to use BIP324 connections. This option is only added in order to make experimentation possible; I don't expect it will be supported long-term (or should even remain in this PR).

Still missing features
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280972174)
> At first sight, I'm not entirely sure why ssKey and ssValue are CDBBatch members when they get resized and cleared in every operation

Haven't check, but if there is more than one operation, they may save a dealloc+alloc?

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1660825982)
(rebased)

💬 sipa commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1660838322)
@stratospher Rebasing on #28196 would let you revive this.

💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1660856546)
In the process of rebasing now.

💬 Ayms commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1660871572)
Not on mobile any longer, then I read the 10 years article, counterparty was right, we are 10 years late, then many switched on eth
The 40 or 80B limit was based on storing a hash or two, surprisingly nobody thought at that time that you must store a signed hash as @ChristopherA and myself wants to do and probably many others, plus some metadata, which is not possible today unless you know a miner, storing a hash or two is of no use, or just leads to a very centralized system, like eth sidechai
...

👍 theStack approved a pull request: "p2p, rpc, test: `addnode` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1557670704)
re-ACK c2349d6a04832d9c96a8746fc588bae8fc85376d

The only change since my last ACK was adapting the first commit's subject (s/getpeerinfo/addnode/). Sorry for late re-review, I must have missed the force-push two months ago.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281063486)
Done

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281063610)
Added a comment

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281063722)
Added a comparison

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1660955989)
> I still suggest changing fc810c1 so that dumpwallet does not use the read-only format by default, and instead change the tests use it.

Since dumping should be a read-only operation, I think it's better to use the read-only database since it definitely won't write to it like BDB does. Also it provides a good mechanism for testing.

💬 fjahr commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1660993936)
Post-merge code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9

I also synced and kept a node running with the status of my previous ACK and didn't see anything unusual.