💬 mzumsande commented on issue "Verify AssumeUTXO snapshot hashes during full validation as well":
(https://github.com/bitcoin/bitcoin/issues/32377#issuecomment-2851372290)
Removing the existing checkpoints is something that has been done to a large part for ideological reasons (devs shouldn't prescribe / hardcode what the "correct" chain is), so introducing something similar back in (and not just historical but new ones) just for the sake of AssumeUxto would be quite contentious I think - I view AssumeUtxo as a nice feature, but a second-class citizen to normal IBD.

I guess if we'd just log a warning in case of a mismatch there is no harm (except for the added t
...

💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2073676011)
Oh right my bad should be `InitWarning(_("Warning: ..."))`!

Anyway yes you should be able to do so. You have to specify the expected `stderr` like:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/wallet_create_tx.py#L72

By default is set to an empty string:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/test_framework/test_framework.py#L607

💬 Sjors commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2851385717)
Indeed I also saw the `chaintate` dir grow after 1 hour, will keep an eye on it.

💬 l0rinc commented on issue "Verify AssumeUTXO snapshot hashes during full validation as well":
(https://github.com/bitcoin/bitcoin/issues/32377#issuecomment-2851395816)
> second-class citizen to normal IBD

Agree, IBD would validate that the AssumeUTXO hashes are correct - lending its credibility to the quicker alternative.

✅ polespinasa closed a pull request: "rpc, logging: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31177)

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2851418794)
I will close this PR for the moment as I don't think I will get enough ACKs with the current approach.
Will be happy to review new approaches to solve this.

💬 Eunovo commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2851542917)
While testing this, I observed a decline in performance as the number of threads increased. I'm not sure why this happens, but I suspect these play a role:
- Doing batch-verification work within the critical section https://github.com/bitcoin/bitcoin/pull/29491/files#diff-0f68d4063c2fb11000f9048e46478e26c0ea2622f7866d00b574f429c3a4db61R151
- Copy operations in https://github.com/bitcoin/bitcoin/pull/29491/files#diff-612abe4a74f2e9f6903cebff057d47bfe4f8a57507c175743f4b62fde0d3cc58R95-R99

I m
...

💬 caesrcd commented on issue "Apparently CJDNS network does not work with Tor on mainnet.":
(https://github.com/bitcoin/bitcoin/issues/24450#issuecomment-2851546390)
I’ve been studying the multi-network support in Bitcoin Core (Clearnet, Tor, I2P, CJDNS), especially from a resilience and redundancy perspective. I’d like to raise a concern regarding the current behavior of routing CJDNS through the general `-proxy` setting.

While it's understandable that `-proxy` applies broadly to IPv4, IPv6, and CJDNS, this design unintentionally breaks a core benefit of multi-network support: **redundancy through independence**.

Unlike Tor or I2P, which are anonymity-foc
...

💬 miketwenty1 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851569165)
Concept ACK.

I believe this is a marginal improvement over #32359. It's better to err on the side of a normal deprecation path with more knobs rather than fewer, especially since this knob (`-datacarriersize`) was previously available. Its removal has been a specific point of controversy, explicitly described by some as core compelling speech. However, this conceptual change will likely affect only a small percentage of previous NACKers, since the intent and outcome seem clear and unchanged i
...

💬 ryanofsky commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2851586658)
Concept ACK.

I think I agree with @purpleKarrot that it is bad to override CMAKE_ variables, and in general the build does too much overriding (which I've mentioned before in places like https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329412411).

But it seems like the PR is moving in the right direction and making a strict improvement by overriding less aggressively than before. I think hebasto's point about wanting CMAKE_BUILD_TYPE=Release (and release builds in multi-config
...

💬 fjahr commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2851688561)
> While testing this, I observed a decline in performance as the number of threads increased.

Can you share the setup for these benchmarks so I can test them? Thanks!

👍 ryanofsky approved a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2815487111)
Code review ACK de7b5eda7ef04e875850fe594de2a89992b7bd46. No significant changes since the last review other than a rebase and a CI fix.

Left some suggestions that I think could make the PR a little easier to understand and review, but they are not important so feel free to ignore.

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073864891)
In commit "wallet: Remove chainStateFlushed" (2d72a8894cfb609ee616b098e165c722183ba325)

Would it be easily possible to squash this commit and the earlier commit "wallet: Remove WriteBestBlock from chainStateFlushed" (35b636469983fdcd5f8e62448f4733effe596e41) together?

I feel like that wouild make the PR easier to understand because the earlier commit doesn't provide any rationale while the commit message here is a little misleading because the commit at this point is only removing dead cod
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073831535)
In commit "wallet: Replace chainStateFlushed in loading with WriteBestBlock" (8b180358fb4c0585f2d9b8c4239d99a98c1fd663)

Not important, but if a goal of this PR is to try to keep m_last_block_processed and bestblock closer in sync, seems like it would be good to call SetLastBlockProcessed here instead of WriteBestBlock.

Same suggestion also applies to AttachChain below, but that WriteBestBlock to SetLastBlockProcessed replacement is already made in a later commit 08668272030bfcffc78efd77b64
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073849511)
In commit "wallet: Write best block record on unload" (eceb848494ea24d3686a9ff9a2d25b657cce9d6e)

Current change should be ok because it's ok if locator is a little behind, but this doesn't seem like most ideal place to write the best block because notifications could still be incoming at the point. A better place would seem like the FlushAndDeleteWallet function before the [Flush](https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/wallet/wallet.cpp#L239) call
...

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2851833591)
Same
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
164958d8c5292ef876e3454725888a488565a414af690a568caedae31215db6c guix-build-b074ae1b4f52/output/dist-archive/bitcoin-b074ae1b4f52.tar.gz
145be7d02fd3d967351ad5944ffe4ceb885e3f7bed6677be657b1c0aa125b40c guix-build-b074ae1b4f52/output/x86_64-w64-mingw32/SHA256SUMS.part
2794ba8c24e64ca1068e04b73748e1406df51089a149a8b0a70637d0bf8e120a guix-build-b074ae1b4f52/output/x
...

📝 theStack opened a pull request: "test: refactor: overhaul (w)txid determination for `CTransaction` objects"
(https://github.com/bitcoin/bitcoin/pull/32421)
In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
* removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997). This change
...

🤔 vasild reviewed a pull request: "test: add test for malleated transaction with valid witness"
(https://github.com/bitcoin/bitcoin/pull/32385#pullrequestreview-2815650001)
Concept ACK, thank you for looking into this!

💬 vasild commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073927377)
I can imagine that malleating a transaction does not require the private key and a re-sign.

Currently there is this function:

```py
def create_malleated_version(self, tx)
```

which fills garbage. Could it be extended to take a boolean argument, indicating whether to fill with garbage like now, or flip `s` to `ORDER - s`? For this it would have to extract `s` like done on line 97 here:

https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/t
...

💬 vasild commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073931876)
Isn't multisig kind of out-of-scope for creating a valid malleated transaction? I don't mind some extra tests, but those might make this PR more difficult to review. Maybe omit those if there is no review interest for some time.