💬 willcl-ark commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3442610839)
Concept ACK.

I have picked this commit into my master branch at https://github.com/willcl-ark/bitcoin along with a CDash config (and a nightly rebase job) so that I can have [my nightly CI jobs](https://github.com/willcl-ark/bitcoin-core-nightly) upload to a demo cdash instance I configured: https://my.cdash.org/index.php?project=core

Seems to work pretty nicely so far :)

The cdash is currently open to all submissions so if anyone else would like to submit jobs, then feel free.

💬 pythcoiner commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3442639256)
reACK [fb72cc3](https://github.com/bitcoin/bitcoin/pull/33135/commits/fb72cc33be570d12fc6b76146fc89047e58f5aaf)

💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-3442728809)
@Sjors I would have preferred I closed this if I deemed it unlikely to succeed

I think the current value is a historical factoid that will cause confusion in the future, especially if something like the GCC softfork happens, but I also don't feel like it's worth spending too much time on.

💬 maflcko commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3442770334)
> CDash

Nice. I think all of this sounds great, but I still haven't seen the larger picture, as the cross-tests and functional tests are left out (https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3340123470), among many other things.

Again, not a blocker for this pull, and I am happy to continue discussion elsewhere. Though, generally my thinking is that the dashboard should not discriminate against tests written in other programming languages, or even tests written outside the t
...

💬 maflcko commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-3442805567)
I don't think there is anything wrong with the patch here, but there is no immediate feature or known use-case attached to it, and it is "just" a policy change. So I think it is fine to not rush it and revisit in a few years, possibly even only after GCC-active and not before it, but not strong opinion.

💬 maflcko commented on issue "ci: windows-native-dll-vcpkg-* cache does not work?":
(https://github.com/bitcoin/bitcoin/issues/33685#issuecomment-3442829355)
> Is this what is expected on Windows with this binary cache too? I am afraid I am so unfamiliar with Windows that I don't know.

Yeah, I am not familiar with GHA, nor Windows, so I can't really help here either.

Just for reference, it seems to be back at being broken: Cache restore, but slow Generate:

https://github.com/bitcoin/bitcoin/actions/runs/18778024349/job/53577115580?pr=33677#step:7:211

💬 maflcko commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-3442845837)
> FWIW it's not really fixed, [#33674](https://github.com/bitcoin/bitcoin/pull/33674) just documents the workaround instruction.

I don't think another fix is possible, as explained in the linked llvm commit above:

> It is difficult to make the
mappings any larger, given the 44-bit pointer compression.

💬 ramayananarju-creator commented on issue "Async Payjoin":
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3442974400)
Just read through this Paybis guide on how to [buy crypto with bank account ](https://paybis.com/buy-bitcoin-with-bank-account/) Super practical if you’re in the U.S. and prefer not to use cards or third-party apps. The process sounds smooth and secure, and I appreciate how they outline each step clearly. It’s nice to see a platform focusing on accessibility while keeping fees transparent. Great resource for beginners who want a simple, bank-linked way to buy Bitcoin or other coins.

💬 maflcko commented on pull request "ci: Doc ASLR workaround for sanitizer tasks":
(https://github.com/bitcoin/bitcoin/pull/33674#discussion_r2460317997)
Yeah, seems preferable to always use the long option name for self-documenting code to avoid confusion. Can be changed in a follow-up, as this is merged.

💬 Raimo33 commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#issuecomment-3443041722)
I've noticed that the `CCoinsCaching` benchmark is a bit deceiving. What we are seeing in this PR is not a speedup in actual coin caching, but rather a speedup in `AreInputsStandard()`

<details>
<summary>benchmark code</summary>

```c
static void CCoinsCaching(benchmark::Bench& bench)
{
ECC_Context ecc_context{};

FillableSigningProvider keystore;
CCoinsView coinsDummy;
CCoinsViewCache coins(&coinsDummy);
std::vector<CMutableTransaction> dummyTransactions =

...

💬 Sjors commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-3443124185)
NACK

If the interface use inside `getblocktemplate` RPC is actually a problem, i.e. the extra copy takes significant time compared to JSON serialisation), then someone can open a PR to undo that.

The `checkBlock()` and `getBlock()` (template) IPC methods are currently indrectly tested through the `getblocktemplate` (`template`) RPC. We don't want to lose that coverage, but since recently have direct test coverage for the IPC interface in https://github.com/bitcoin/bitcoin/blob/master/test/
...

💬 BenWestgate commented on pull request "wallet: add codex32 argument to addhdkey":
(https://github.com/bitcoin/bitcoin/pull/32652#issuecomment-3443135285)
> I still haven't gotten around to trying the paper booklet instructions. If and when I do that, I'll probably review this. But no idea when that is.

From BIP-0093:
> ...hand computation is optional, ... and implementers do not need to be concerned with this possibility.

To encode some codex32 strings to import without the paper booklet there are libraries in [Rust](https://github.com/BlockstreamResearch/codex32/tree/master/reference/rust-codex32/src), [Ruby](https://github.com/azuchi/cod
...

💬 kevkevinpal commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2460439758)
ahh I didnt realize that Podman would fail with empty string

ok np what you have currently looks best then

💬 kevkevinpal commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3443144320)
ACK [5555bce](https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3)

Retrying building image on failure makes sense to me, and the refactor looks good

💬 stickies-v commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2460414539)
Apologies for mostly coming with stop energy here, but I think I'm approach NACK on 30b190eebab9df9273586e0a4b6462847883d9d1, and I would prefer dropping the commit. While the duplicated header calculation and flag setting is wasteful, I don't think we should complicate our validation logic further for the very rare case that is `invalidateblock`.

I don't have a suggested alternative yet, but it seems to me that reorganizing this code into different, more natural functions might be an alterna
...

💬 stickies-v commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2460211532)
Default values are best defined in the header file

💬 rkrux commented on issue "Internal bug detected: FinalizeAndExtractPSBT(psbtx_copy, mtx)":
(https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3443211089)
Is this bug still there? I am not able to reproduce it.
Upon passing an incorrect sighash to `descriptorprocesspsbt`, I get the following error instead:

```zsh
Specified sighash value does not match value stored in PSBT
```

I think v30 has resolved it because of this commit: https://github.com/bitcoin/bitcoin/commit/ee045b61efc1479c1866b786661ef39a863677d0

🤔 rkrux reviewed a pull request: "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures"
(https://github.com/bitcoin/bitcoin/pull/33014#pullrequestreview-3376793862)
I don't think the corresponding bug is still present after v30, see: https://github.com/bitcoin/bitcoin/issues/32849#issuecomment-3443211089

💬 rkrux commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#discussion_r2460512713)
```diff
- // Check whether or not all of the inputs are now signed
+ // Check whether or not all of the inputs are now correctly signed
```

💬 rkrux commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#discussion_r2460510581)
It's discouraged to use bitwise operators on booleans: https://stackoverflow.com/questions/24542/using-bitwise-operators-for-booleans-in-c

```diff
- complete &= PSBTInputSignedAndVerified(psbtx, i, &txdata);
+ complete = complete && PSBTInputSignedAndVerified(psbtx, i, &txdata);
```