👋 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? :/

💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1650649661)
> If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.

I've tried to port our MSVC build from Cirrus CI to GitHub Actions CI and faced an issue: https://github.com/actions/runner-images/issues/7832. A suggested [workaround](https://github.com/widelands/widelands/pull/6007) did not help. I'm going to investigate it shortly.

💬 achow101 commented on pull request "util: Don't derive secure_allocator from std::allocator":
(https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1650663419)
ACK 07c59eda00841aafaafd8fd648217b56b1e907c9

🚀 achow101 merged a pull request: "util: Don't derive secure_allocator from std::allocator"
(https://github.com/bitcoin/bitcoin/pull/27930)

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1650921567)
After thinking/working through further PRs, I'm beginning to wonder if it isn't better if these classes support incremental computation (i.e., the entire message/packet to be encrypted/decrypted isn't required to be presented in a one (or two) contiguous block(s) of memory, at a single point in time).

The advantage would primarily be a slightly lower latency when receiving a large P2P message. As is, the entire message needs to be received before any decryption can take place. With an increment
...