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

💬 achow101 commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650622058)
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

🚀 achow101 merged a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113)

📝 ismaelsadeeq opened a pull request: "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain"
(https://github.com/bitcoin/bitcoin/pull/28157)
This PR Follow up comments from [#27622](https://github.com/bitcoin/bitcoin/pull/27622)

It test that the new `regtest-only` option `acceptstalefeeestimates` is not supported on [main, signet and test chains](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235218268), removes an unnecessary [comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r1235204323), and update fee estimator `MAXFILEAGE` [description comment](https://github.com/bitcoin/bitcoin/pull/27622/files#r123388731
...

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650639740)
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

One follow-up could be test coverage for the error case.

@fanquake could you provide more info on how to reproduce your data (e.g. regular build or debug build, the ./configure used, etc.)

I'm not surprised that extraction in the push at d87edf3 didn't make a difference, as the include directives in that version were not optimized for the change. The implementation at https://github.com/jonatack/bitcoin/commits/2023-07-extract-sighashtype-and
...

💬 luke-jr commented on pull request "contrib: Add script to colorize logs":
(https://github.com/bitcoin/bitcoin/pull/26052#issuecomment-1650641281)
Why was this deleted? :/