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

💬 MarcoFalke commented on pull request "test: fix `feature_addrman.py` on big-endian systems":
(https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1651013529)
tested as well, so my recommendation would be to merge this before #https://github.com/bitcoin/bitcoin/pull/28087

💬 MarcoFalke commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274407349)
unrelated in 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da: I think peermanager options have nothing to do with addrmanager options, so this could be moved down, closer to where it is used.

💬 MarcoFalke commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651027508)
lgtm ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da