💬 willcl-ark commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1660735316)
> [ @willcl-ark You could try to inspect the binary after compilation to see how the compiler optimized it (and what difference there is to your version) ]

I did take a rough look on my machine (and on [godbolt](https://godbolt.org/z/M9a7WG5s3)), but I don't think I have the necessary super-powers needed to really dissect why my (very naive and likely badly implemented) impl. didn't pay off.

From what I can see, it doesn't appear that clang16 is auto-SIMD-ing your version, but my SIMD vers
...

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280886196)
In commit "RPC/Wallet: Convert walletprocesspsbt to use options parameter" (baf99a9c789779e89e7684a550e16cf1fb7ce862)

I think `OBJ` should be replaced `OBJ_NAMED_PARAMS` so generated documentation is consistent with other RPC methods that take options parameters.

Replacing `OBJ` with `OBJ_NAMED_PARAMS` will also allow `sighashtype` and `bip32derivs` and `finalize` options to be continue to be accepted as named parameters if the backwards compatibility code below accepting them as positiona
...

🤔 ryanofsky reviewed a pull request: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-1557402115)
Light code review ACK baf99a9c789779e89e7684a550e16cf1fb7ce862

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280895729)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)

I think this `arg.m_type` reference on line 730 also needs to be replaced by `argtype`.

If we got rid of the `RPCArg::m_type` member (see other comment) it would avoid this type of bug.

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280898093)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)

I think just calling this new struct member `m_types` would be more consistent with the `m_names` member. Also I think a followup commit should remove the `m_type` member which is now confusing and redundant.

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280912926)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)

It would be good to have unit test coverage for this new code. Should be easy to add a test creating an RPCArg and calling this method in rpc_tests.cpp.

👍 Sjors approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1557474340)
re-ACK e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518

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