✅ maflcko closed a pull request: "rpc: Add validation for invalid taproot signatures in analyzepsbt"
(https://github.com/bitcoin/bitcoin/pull/33360)

💬 maflcko commented on pull request "rpc: Add validation for invalid taproot signatures in analyzepsbt":
(https://github.com/bitcoin/bitcoin/pull/33360#issuecomment-3334326637)
Closing for now. This is LLM generated and obviously wrong (the tests fail), and the author does not seem to be working on it (no activity since this was opened 2 weeks ago)

✅ maflcko closed a pull request: "Fix #25980: Validate transactions in combinerawtransaction"
(https://github.com/bitcoin/bitcoin/pull/33361)

💬 maflcko commented on pull request "Fix #25980: Validate transactions in combinerawtransaction":
(https://github.com/bitcoin/bitcoin/pull/33361#issuecomment-3334330381)
Closing for now. This is LLM generated and obviously wrong (the tests fail), and the author does not seem to be working on it (no activity since this was opened 2 weeks ago)

💬 maflcko commented on pull request "Fix #25980: Validate transactions in combinerawtransaction":
(https://github.com/bitcoin/bitcoin/pull/33361#issuecomment-3334346015)
In the future, instead of creating competing pull requests, it would be better to just review the existing pull request with any feedback you may have.

💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3334435069)
Interesting side note: Looks like most unit tests are minimally faster on Alpine, except for the secp tests:

https://github.com/bitcoin/bitcoin/actions/runs/18007497948/job/51231174310?pr=33480#step:9:3415:

```
148/150 Test #4: secp256k1_noverify_tests ............. Passed 38.88 sec
149/150 Test #5: secp256k1_tests ...................... Passed 57.62 sec
```

https://github.com/bitcoin/bitcoin/actions/runs/18007193787/job/51230198016#step:9:2625 :

```
145/150 Test #
...

💬 RandyMcMillan commented on pull request "rpcconsole: display signet challenge":
(https://github.com/bitcoin-core/gui/pull/896#discussion_r2379384218)
I was think of renaming this to challengeToStdString - that seems to be closer to established convention in the code base. Thoughts?

👍 ryanofsky approved a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3268088341)
Code review ACK 5b20d172ca2a46a2b525201b4ff2444f9d415d8c. Just added link to upstream modernize-use-default-member-init bug (thanks for looking into that and reporting) and added new suppressions for capnproto clang-tidy errors since last review

💬 ryanofsky commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379380656)
In commit "clang-tidy: Disable `ArrayBound` check in src/ipc and src/test" (5b20d172ca2a46a2b525201b4ff2444f9d415d8c)

Kind of a shame to need this outside of the IPC directory. Maybe there should be a `src/ipc/test` subdirectory like `src/wallet/test` to keep IPC stuff grouped together.

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379404041)
The same thought occurred to me while working on this PR, but I thought it might be better to defer it to a separate PR. Do you think it’s worth adding another commit to move the tests here?

💬 ryanofsky commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3334508468)
> can also share stack trace later in case you feel it's still valuable

Would appreciate that if you get a chance. I do suspect this is related to #33387 but would be nice to have some confirmation or ideally a reliable way to reproduce.

Thanks for reporting the bug in any case, and if doesn't seem worth collecting more debug information, it would be ok to close this

💬 janb84 commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3334556327)
So Centos as CI task was not added to give good RHEL distro / Enterprise Linux support ?

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3268185464)
Code review ACK 7dd85d13e22f14940cce9ed9a5bbc2afc5c5c2f4. Only change since last review is applying int->size_t and documentation tweaks. Thanks for the update!

💬 ryanofsky commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379461800)
re: https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379404041

> The same thought occurred to me while working on this PR, but I thought it might be better to defer it to a separate PR. Do you think it’s worth adding another commit to move the tests here?

I was thinking of it as a followup for a future PR, but I'm actually not sure how big the change would be and whether it might require code changes. Maybe if it's a small change it would be easy to make here. Whatever your prefe
...

💬 Crypt-iQ commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#issuecomment-3334611538)
crACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596

💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3334691377)
> So Centos as CI task was not added to give good RHEL distro / Enterprise Linux support ?

No, as mentioned in the pull description. For reference, the history was:

* https://github.com/bitcoin/bitcoin/pull/17635: Add centos 7 CI to check "old packages".
* https://github.com/bitcoin/bitcoin/pull/17900: Drop the "old packages" and use depends (32-bit).
* https://github.com/bitcoin/bitcoin/pull/31651: Use native depends instead of 32-bit.
* https://github.com/bitcoin/bitcoin/pull/31593: B
...

💬 HowHsu commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3334805323)
> I think it would make sense to support XOR for loadblock if we're assuming the chain has data that can trigger bad things

Hi Luke, what do you mean by "assuming the chain has data that can trigger bad things"

💬 purpleKarrot commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3334810848)
Concept NACK. I think this development goes into the wrong direction.

> * With sanitizers enabled, it is one of the slowest targets, often taking several minutes.

The test could be sharded to allow better parallelization. (Nit: "target" is the wrong terminology here)

> * There is no insight from ctest into how long each individual sanity check takes.

CTest can provide much more detailed statistics, with duration, memory usage and custom measurements. It just needs to be configured p
...

💬 HowHsu commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3334812415)
> I think we should try it ourselves before recommending it in the documentation

I'll try that later when I have some time for it, though the function of the script is already clear enough for me by reading the code.

💬 HowHsu commented on pull request "index: handle case where pindex_prev equals chain tip in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2379604229)
> Not sure what you mean, I prefer having a test that I can run with and without the fix to debug both cases to understand the change better. Otherwise this just looks like a random change that we can just revert and the CI wouldn't even catch it.

Again, why do we write a test for a code clean change?