👍 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

💬 theuni commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1650549473)
Concept ACK. The patch we've taken from Debian is described as:

> replacing the aligned instruction with the unaligned one the hacky patch below got created, just replacing vmovapd by vmovupd. Not considering any side effects and maybe other instructions with alignment requirements.


Which appears to be what the new assembler option does as well. Using the supported switch is much better than the hacky patch.

What's up with the single aligned case though? Is that maybe coming from some
...

💬 theuni commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1650550572)
Any reason not to add it to our configure directly rather than guix?

💬 luke-jr commented on pull request "rpc, docs: Add note for commands that supports only legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/25680#issuecomment-1650553050)
@achow101 Why was this merged?

I'm not sure suggesting `combo()` is a good idea. Isn't combo only for having a migration path, not something we want people to actually use?

💬 achow101 commented on pull request "rpc, docs: Add note for commands that supports only legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/25680#issuecomment-1650569833)
While I don't think we should necessarily encourage people to use `combo()`, I don't think it's bad to mention it either. in the case of `importpubkey` and `importprivkey`, `combo()` with `importdescriptors` does replicate their behavior so it isn't incorrect. I don't really care either way whether it's mentioned or not.

💬 stickies-v commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1274129104)
Given that we use this value [to size a vector](https://github.com/bitcoin/bitcoin/blob/e35fb7bc48d360585b80d0c7f89ac5087c1d405e/src/net_processing.cpp#L1654), that could be sensible, but of course, arbitrary upper limits bring their own set of problems. I'll look into it more, thanks!

👍 hebasto approved a pull request: "suppressions: note that `type:ClassName::MethodName` should be used"
(https://github.com/bitcoin/bitcoin/pull/28147#pullrequestreview-1546483645)
ACK d0c6cc4abe42163aaf081a969d2c449785563ba2

💬 theuni commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1650579987)
I'm not sure that I love using a subsystem's opts as a cache for init vars. But the consistency reasoning makes sense, so ~0 on that.

💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1650601548)
> > > Just curious, did you try using `chattr +i` to set blockfile as **immutable** ? May break other stuff, I don't know...
> >
> >
> > Bold strategy, Cotton let's see if it pays off: [cd394b6](https://github.com/bitcoin/bitcoin/commit/cd394b637f98b79dc3a4623ec4b970137ee2c589)
>
> _Narrator: "It didn't."_ https://cirrus-ci.com/build/4907092587315200
>
> Seemed like a good idea though but bitcoind is still able to open the "immutable" files in write mode thinking

I've just tried do
...

💬 jonatack commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1650609478)
@achow101 I found the CI environment changes useful for making https://github.com/bitcoin/bitcoin/pull/28116 work as well.