💬 MarcoFalke commented on pull request "test: Avoid intermittent issues due to async events in validationinterface_tests":
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1650265913)
> Using ChainTestingSetup should be faster?

It is doing less setup, so it may also be faster.

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1273901955)
```suggestion
{RPCResult::Type::BOOL, "silent_payment", "whether this wallet supports silent payments"},
```

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1273902470)
```suggestion
{"silent_payment", RPCArg::Type::BOOL, RPCArg::Default{false}, "Experimental. Indicates that the wallet supports Silent Payments. This means a more complex scanning logic."},
```

👍 john-moffett approved a pull request: "util: Don't derive secure_allocator from std::allocator"
(https://github.com/bitcoin/bitcoin/pull/27930#pullrequestreview-1546065248)
ACK 07c59eda00841aafaafd8fd648217b56b1e907c9 Reviewed and tested. Performance appears unaffected in my environment.

💬 luke-jr commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1650329917)
Concept ACK, but I'm not too sure about the `Wtxid` type. It's literally just a hash of the transaction, not really an identifier per se.

💬 luke-jr commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1650338677)
Agree with @mzumsande and @naumenkogs. Maybe not the same PR if it's substantially different, but the new eviction logic should be merged before this fix.

💬 luke-jr commented on pull request "include verbose debug messages in testmempoolaccept reject-reason":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-1650342544)
Why not pass the details in `m_debug_message` and add that as a new field?

💬 luke-jr commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1273943679)
I don't think there is any currently-supported case where `template_request` can be omitted. For a new template, the client must indicate it supports segwit with the rules key. For a proposal, there must be data.

💬 furszy commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1273952235)
yep. Pushed.

💬 luke-jr commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1650366541)
Concept ACK, but the description has some issues:

>Obviously, with this much support of full-rbf, arguments against it on the basis that unconfirmed transactions are safe are now invalid.

It was always invalid.

>Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for consistency with miners.

This is backward. Miners are incentivi
...

💬 jonatack commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1650366956)
re-ACK 07c59eda00841aafaafd8fd648217b56b1e907c9 no change since my previous ACK apart from squashing the commits

💬 luke-jr commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#issuecomment-1650381312)
Concept NACK, I think. Wallet encryption is not going to help you if someone steals your wallet, only if someone gains very temporary access to your keyboard/mouse only. I don't think this full-encryption you're proposing has a real use case.

(What might make sense is to support fully encrypting backups.)

👍 TheCharlatan approved a pull request: "refactor: consistently use ApplyArgsManOptions for PeerManager::Options"
(https://github.com/bitcoin/bitcoin/pull/28148#pullrequestreview-1546218973)
ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da

👍 Crypt-iQ approved a pull request: "test: Avoid intermittent issues due to async events in validationinterface_tests"
(https://github.com/bitcoin/bitcoin/pull/28150#pullrequestreview-1546322142)
tACK faca9a3d5a6887517d02b994a43d0e1101b718bc

🤔 TheCharlatan reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1546251193)
ACK 6e28ff97a99519ec8b50123bc1177084bba68f96 (besides the two small comments)

I'm sure I could spend many hours more verifying the parser, but at this point I'd like to encourage other reviewers to take a look. I think if this is merged soon and given the opportunity to be fuzzed against a large corpus of both random seeds, bdb databases, and valid wallets we could also gain some confidence in the code.

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1273993906)
This would be more readable if the `main` bytes were stuffed into a constant somewhere.

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1274005925)
Can you add a comment explaining why we check this?

👍 theStack approved a pull request: "test: Ignore UTF-8 errors in assert_debug_log"
(https://github.com/bitcoin/bitcoin/pull/28035#pullrequestreview-1546364780)
utACK fa3d72960bc86319aa8b838e3df4e833f845c71f

Changes look correct to me and should fix #28030.
(Background information for other reviewers: `wait_for_debug_log` was changed to open the debug file in binary mode in PR #25294)

🤔 dergoegge reviewed a pull request: "net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1546357513)
Concept ACK

💬 dergoegge commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1274049289)
Wondering if we can come up with a more sane upper limit for both of these?