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

📝 brunoerg opened a pull request: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153)
This PR adds target for `{Legacy}ScriptPubKeyMan`. I'm working on a descriptor one and will do it in a separate file. I tried to focus here on functions that we use directly and we may have in some unit tests.

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

Thanks for the follow-up!

💬 luke-jr commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#discussion_r1274077350)
Kinda ugly to do the increment here. Maybe make the loop normal, and check for `it != mapLocalHost.begin()` at the start of it?

📝 theStack opened a pull request: "test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28154)
This PR is a simple follow-up for #28025. It introduces a `signing_input_segwitv0` helper in order to deduplicate the following steps needed to create a segwitv0 ECDSA signature:
1. calculate the `SegwitV0SignatureHash` with the desired sighash type
2. create the actual digital signature by calling ECKey.sign_ecdsa on the signature message hash calculated above
3. put the DER-encoded result (plus sighash byte) at the bottom of the witness stack

📝 sr-gi opened a pull request: "Improves addnode / m_added_nodes logic"
(https://github.com/bitcoin/bitcoin/pull/28155)
## Rationale

Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.

### Connecting to the same node more than once

In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different way
...

👋 brunoerg's pull request is ready for review: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153)

💬 sr-gi commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1650541120)
I've purposely split this into three commits according to each of the fixes so they can be discussed, and considered independently. That way it'll be easier to remove any if the change does not reach enough consensus.

💬 stickies-v commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1274110667)
Ah thanks, fixed in aa89e04e07ca9ff51b1d7d310a11821c6ad963cf