💬 jonatack commented on pull request "Implement `CCoinsViewErrorCatcher::HaveCoin` and check disk space periodically":
(https://github.com/bitcoin/bitcoin/pull/26331#issuecomment-1810843435)
Post-merge concept ACK. Some kind of test coverage would be good.

💬 ismaelsadeeq commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1810845253)

> It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.

Previosuly we always got the `Transaction has too long of a mempool chain` message but with this PR we now get the node unconfirmed parents limit.
[checkChainLimits](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/node/interfaces.cpp#L703) is sending a package with one Internally created transaction and its `v
...

🤔 jonatack reviewed a pull request: "build: remove `-bind_at_load` usage"
(https://github.com/bitcoin/bitcoin/pull/28783#pullrequestreview-1730409395)
Tested on ARM64 M1 Max with macOS Ventura 13.6 and the `bind_at_load` warnings are now gone. Thank you for fixing this.

💬 brunoerg commented on pull request "test: add stress test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810888521)
> Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?

Sure! "Stressing" is basically test bucketing logic in a situation that we try to add in the new table:
- Addresses from more distinct ASs than the number of available buckets (2000 is a little bit less than 2x the number of buckets. I chose this number to not make the test slower, however, if we migrate this test to `contrib`, w
...

💬 jonatack commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1810906067)
Here is what happened when I tried to use this according to the added documentation. Any suggestion?

```
jon|master =:~/bitcoin/bitcoin$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run
zsh: command not found: cargo

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install cargo
Running `brew update --auto-update`...
==> Auto-updated Homebrew!
==> Updated Homebrew from 75e8376816 to 8df40f4a41.
No changes to formulae or casks.

Warning: No available f
...

📝 TheCharlatan opened a pull request: "bench: Update nanobench to 4.3.11"
(https://github.com/bitcoin/bitcoin/pull/28877)
The newest version fixes the false positive `* Turbo is enabled, CPU frequency will fluctuate` warning on AMD CPUs. The file was directly taken from the release page: https://github.com/martinus/nanobench/releases/tag/v4.3.11.

Other changes from the release notes:

* Check for failures in parseFile(), perf events tweaks by @tommi-cujo in https://github.com/martinus/nanobench/pull/84
* Workaround missing noexcept for std::string move assignment by @tommi-cujo in https://github.com/martinus/
...

💬 fanquake commented on pull request "build: Fix LTO functionality":
(https://github.com/bitcoin/bitcoin/pull/28876#issuecomment-1811062710)
In case anyone is unsure, the --enable-lto option, as-is, does work, no broken binaries etc (and we use (thin) LTO in oss-fuzz).

Changes 1 & 3 here don't affect the compiled binary. The compiler will eat any duplicate -flto flags; the change to the -Werror flag is correct, but also does not affect the compiled binary.

Passing the flags through to libsecp is an improvement, and I'm pretty sure that wasn't done originally, because when the --enable-lto option was implemented, we didn't have a cl
...

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