💬 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?

🤔 janb84 reviewed a pull request: "ci: Turn CentOS config into Alpine musl config"
(https://github.com/bitcoin/bitcoin/pull/33480#pullrequestreview-3268463472)
Concept ACK fa6b2e9efece2d728bdc257c36c95db03e1a7bc4

This PR introduces more libc diversity in the CI pipeline (in the form of using Alpine), which is welcome.

Not completely agreeing with this PR sentence;
> "So basically, the centos task is similar to all the Ubuntu/Debian CI tasks, possibly with some packages named slightly differently. "

Yes and No; (Risk of being too much of a nitpicker.)

- Yes from a Libc perspective; Centos is glibc and Debian is also glibc

- No from
...

💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3334874591)
> Why can't CTest be used in the Windows-cross CI task? That should be fixed.

It is documented right in the file: ` # Can't use ctest here like other jobs as we don't have a CMake build tree.`

No one is anyone holding back from fixing that, but personally I don't think it is possible, or only possible with massive code and maintenance overhead.

> Instead of migrating away from CTest, we should fully embrace i.

If you want to implement all of this in CTest, please go for it. Ho
...

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)
> I was thinking of it as a followup for a future PR...

I agree with that.

> ... but I'm actually not sure how big the change would be and whether it might require code changes.

Something like a978c6bdb41fd3f8461908c30bb409a26fa62d47 in https://github.com/hebasto/bitcoin/commits/250925-llvm-tidy/.

💬 l0rinc commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3334938418)
> It is already tested in `test/functional/feature_loadblock.py`, no?

That's where I found it, but to see if it solves the original problem for why this PR was opened, it would help to have personal experience with the script so that we can document it such that others don't need to browse the tests. I'm also fine with anyone else having experience with the script suggesting a useful help text.