💬 TheCharlatan commented on pull request "bench: Update nanobench to 4.3.11":
(https://github.com/bitcoin/bitcoin/pull/28877#issuecomment-1811066329)
@martinus let me know if you'd prefer authorship of the commit :)

💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1811067757)
I'd suggest fixing your brew / cargo / rust installing. It looks broken, i.e:

error: process didn't exit successfully: `rustc -vV`

Just running the compiler doesn't work.

💬 helpau commented on issue "Sync slow":
(https://github.com/bitcoin/bitcoin/issues/26063#issuecomment-1811065968)
I found the same problem, and started looking for the causes. I found a large block connection time [bench] - Connect total: 16234.98ms .
My configuration: Windows 10 22H2 x64, Bitcoin Core 25.1.0, Xeon E5-2670, 16 GB RAM, 1TB HDD SATA III drive with 64 MB cache, 90% free, NTFS with 4 KB cluster size
Bitcoin Core configuration: prune 2 GB, dbcache 12000 MB, -1 threads
My steps:
1)Wait for IBD to complete
2)Shut down Bitcoin Core
3)Defragment the disc
4)Turn on Bitcoin Core
5)Wait an hour
...

💬 helpau commented on issue "Bitcoin Core very slow sync":
(https://github.com/bitcoin/bitcoin/issues/27134#issuecomment-1811074530)
Please reopen the issue, I will try to give logs with lock conflicts later. Probably related #776

💬 AdamISZ commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1811075330)
> > Is it possible to support descriptors that include only the "aggregated xpub" and not every key as in the proposed `agg()` expression, leading to a O(1) size instead of O(number-of-keys)?
>
> The aggregated pubkey will look like any other pubkey, so it is trivial to substitute it in to the descriptor if you wish to do so. However this is lossy as you will lose the information about the pubkeys that are involved in that aggregated pubkey, and that information is necessary for spending. It
...

💬 martinus commented on pull request "bench: Update nanobench to 4.3.11":
(https://github.com/bitcoin/bitcoin/pull/28877#issuecomment-1811075827)
@TheCharlatan no need, thanks for updating!

💬 helpau commented on issue "Sync slow":
(https://github.com/bitcoin/bitcoin/issues/26063#issuecomment-1811077587)
Please reopen the issue, I will try to give logs with lock conflicts later. Probably related #776

💬 maflcko commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1811164649)
The `cargo fmt` and clippy part is just for writing new code and not strictly needed for just running code. You should be good to skip over it for now. However, if you want to write code, it may be better to look into getting it to work.

💬 maflcko commented on pull request "test: Generate coverage report without running tests":
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1811198917)
Right. I am saying this is certainly a behavior change. Previously one could just type `./configure ... && make && make cov` to get the both reports. Now, something else is needed, likely a longer command, so it would be good to mention this in the documentation, or pull request description, or anywhere. Otherwise, everyone is left on their own to figure out the change in behavior.

💬 jonatack commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1811202331)
Thanks. Resolved (on ARM64 macOS 13.6 Ventura) as follows:

```
$ brew remove rust
$ curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
$ rustc --version
rustc 1.73.0 (cc66ad468 2023-10-03)
$ cargo --list
```

(running `cargo --list` now includes the `fmt` command)

```
$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run
Checking test_runner v0.1.0 (/Users/jon/bitcoin/bitcoin/test/lint/test_runner)
Finished dev [unoptimized + debuginfo] tar
...

💬 maflcko commented on pull request "[26.x] rc3 or finalize":
(https://github.com/bitcoin/bitcoin/pull/28872#issuecomment-1811216180)
> Would suggest backporting

No strong opinion, but I think that backports at this stage should either fix a regression or be clear bugfixes. Other behavior changes are better left for the next release, because they could themselves change behavior in a buggy way, see also the previous comment https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789702441

🤔 theuni reviewed a pull request: "Use serialization parameters for CTransaction"
(https://github.com/bitcoin/bitcoin/pull/28438#pullrequestreview-1730610636)
I tried to step through everything to verify the correctness of the witness vs non-witness choices in the changed code. I agree with all of them.

Only skimmed the tests.

I left a comment about cleaning up `version.h` includes which I believe should be included as part of this PR.

Looks good otherwise.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393167010)
It's kinda a shame to carry this default param forward. But it's also not much better to have a bool at each callsite. An enum class {witness/no_witness} would remove all ambiguity at the callsites, but that may be overkill.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393168493)
Nice, this code is _much_ more straightforward now.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393167335)
Same comment about default bool.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393193011)
If version is no longer used for fileout in these ops, can't we `s/CAutoFile/AutoFile/g` ?

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393187848)
I guess this makes more sense than defining witness-ness for an empty block, but just to clarify...

This is a no-op and would've been a no-op independent of this PR as well, right?

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393181788)
Beautiful :)

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393251131)
I realize this isn't new, but it's pretty icky to go to `gArgs` for something as low-level as serialization. Makes me wonder if we're doing that in any tight loops. Nothing to do with this PR, but it be nice to cache this in a local bool.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393249080)
These can be a `DataStream`s now?