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

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