💬 maflcko commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2595094075)
For reference, multiprocess was turned into i686, because the centos task didn't find one issue, see https://github.com/bitcoin/bitcoin/pull/22923

cc @hebasto

💬 hebasto commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2595113943)
> Turning the task into a native one makes it easier to run it on aarch64 or any other supported architecture.

Mind elaborating "easier to run it on aarch64"?

💬 maflcko commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2595119052)
> > Turning the task into a native one makes it easier to run it on aarch64 or any other supported architecture.
>
> Mind elaborating "easier to run it on aarch64"?

Thanks, replaced "easier" with "possible", because with current master it is not possible to run the task natively. (Since it is not a native task).

📝 fanquake locked a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31667)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2595130819)
Aside from the existential question, it would be useful to have some projects to test this PR against: https://bitcoin.stackexchange.com/questions/125384/who-uses-or-wants-to-use-psbtv2-bip370

💬 dergoegge commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2595132646)
> We already have a target for it, eval_script. It could be adapted...

Yes that would be an alternative to some extend but I want to keep this test to `VerifyScript` as that function uses the flags as well.

> The same applies to signature_checker and eval_script harnesses?

That is possible but I would consider that out of scope for this PR.

🤔 TheCharlatan reviewed a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2552251401)
Reviewed up to the last commit. This looks very nice, I especially like the move of business logic out of the rpc into a more appropriate module. I also think the new place for `m_warning_caches` is much less confusing.

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916411893)
In commit dc37009c621f3ab851e4b78e3a797cb868d1a2ed
Doesn't this now return 144 for any non-mainnet chain? Wouldn't it be more correct to return `return m_chainman.GetConsensus().vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].period;`?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916389216)
Typo nit: Extra whitespace.

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916365131)
In commit 9bc41f1b48b2e0cc6abf9714e860a29989d7809c
Nit: Could make this a constexpr, and rename to indicate its global nature?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916585264)
Can you add a docstring here? I think the semantics of the return type should be explained.

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916483128)
Nit: Fix the indentation here?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1918165052)
In commit af817ebb2af5870c082984e41731569c090ff3fc
Nit: Unneeded whitespace change.

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916603572)
Nit: Select `ACTIVE` and `LOCKED_IN` from the enum class name?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916546718)
Why did you choose `0` here and `2016` for the period?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1918190252)
In commit 49afd714b40115b15ea0ef146b48e9d7a403a596

Can you give some more motivation for splitting this out into its own header, especially since it seems to be introducing a new circular dependency? It also not immediately clear to me why a file called implementation only holds abstract declarations. From what I understand this is supposed to hold declarations that are only relevant for external use by tests.

💬 l0rinc commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2595151977)
Since profilers may not catch these short-lived spikes, I've instrumented the code, loaded the UTXO set (as described the in the PR), parsed the logged flushing times and memory usages and plotted them against each other to see the effect of the batch size increase.

<details>
<summary>txdb.cpp flush time and memory instrumentation:</summary>

```patch
diff --git a/src/txdb.cpp b/src/txdb.cpp
--- a/src/txdb.cpp (revision d249a353be58868d41d2a7c57357038ffd779eba)
+++ b/src/txdb.cpp (revis
...

👍 hebasto approved a pull request: "ci: Turn CentOS task into native one"
(https://github.com/bitcoin/bitcoin/pull/31651#pullrequestreview-2555513010)
ACK fabefd9915802d53d8c83ae66e5cb259196d9422, tested locally on Ubuntu 24.10.

💬 hebasto commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#discussion_r1918212255)
Shouldn't "depends" be mentioned here for consistency with other task names?

💬 dergoegge commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1918215963)
I mostly added that assertion to prevent a buffer overflow in the `memcmp` below, which won't happen as long as the `program` span is 32 bytes in size or larger.

I can see the value in asserting that `program` is an actually a v0 script hash (i.e. 32 bytes exactly). I'll consider changing it if I retouch.