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

💬 instagibbs commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2851921477)
> determining a (w)txid for a CTransaction instance is currently rather confusing and footgunny

I don't disbelieve you exactly as I always have to double-check what I'm calling, but have we seen a series of footguns?

🤔 murchandamus reviewed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2815722815)
I concept ACKed this PR earlier, but after discussing the proposed change for several days, I would prefer if the configuration option were removed at a later time. I still support dropping the limit on count and size of OP_RETURN data payloads.

💬 stratospher commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073973241)
oh included it because high-s transactions are non-standard and I was worried they wouldn't be relayed and we couldn't test private broadcast behaviour.

💬 luke-jr commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851998711)
Concept NACK. Already been over this on multiple PRs. Spamming it with immaterial minor differences won't change the fundamental insanity and malice of the concept.