💬 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
...
(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"
(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
...
(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.
(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?
(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
...
(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
...
(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/.
(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.
(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.
💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3334946430)
> nitpicker
No worries. Happy to adjust the pull description, if you have any suggestions I could take over.
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3334946430)
> nitpicker
No worries. Happy to adjust the pull description, if you have any suggestions I could take over.
💬 l0rinc commented on pull request "index: handle case where pindex_prev equals chain tip in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2379697450)
Not sure what that means, we write tests to tell us when the behavior of the application changed.
You have changed the behavior and no test broke, so there's a lack of test coverage that we should fix to explain why this change is needed. It also helps the reviewers debug the given scenario to understand why this changes is needed - or if there's a different solution that would also solve it, etc.
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2379697450)
Not sure what that means, we write tests to tell us when the behavior of the application changed.
You have changed the behavior and no test broke, so there's a lack of test coverage that we should fix to explain why this change is needed. It also helps the reviewers debug the given scenario to understand why this changes is needed - or if there's a different solution that would also solve it, etc.
🤔 danielabrozzoni reviewed a pull request: "p2p: Use network-dependent timers for inbound inv scheduling"
(https://github.com/bitcoin/bitcoin/pull/33464#pullrequestreview-3268650422)
tACK beb75e48ae1d5771932f427a490c7e1b6c1720d3
I tested locally by adding a log line, and checking that different networks schedule different inv times:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f5f04c7a17..fec1cfcb6b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5716,6 +5716,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
fSendTrickle = true;
if (pto->IsInboundConn()) {
...
(https://github.com/bitcoin/bitcoin/pull/33464#pullrequestreview-3268650422)
tACK beb75e48ae1d5771932f427a490c7e1b6c1720d3
I tested locally by adding a log line, and checking that different networks schedule different inv times:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f5f04c7a17..fec1cfcb6b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5716,6 +5716,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
fSendTrickle = true;
if (pto->IsInboundConn()) {
...
💬 fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2379758795)
This line [fails in the no IPC, i686, DEBUG CI](https://github.com/bitcoin/bitcoin/actions/runs/17962047927/job/51087206906?pr=29675#step:9:3082).
<details><summary>Traceback</summary>
```
Traceback (most recent call last):
File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_framework.py", line 199, in main
self.run_test()
File "/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_musig.py", line 224, in run_test
self.do_test("ra
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2379758795)
This line [fails in the no IPC, i686, DEBUG CI](https://github.com/bitcoin/bitcoin/actions/runs/17962047927/job/51087206906?pr=29675#step:9:3082).
<details><summary>Traceback</summary>
```
Traceback (most recent call last):
File "/home/admin/actions-runner/_work/_temp/test/functional/test_framework/test_framework.py", line 199, in main
self.run_test()
File "/home/admin/actions-runner/_work/_temp/build/test/functional/wallet_musig.py", line 224, in run_test
self.do_test("ra
...
💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3335108541)
Also confirmed that the gcc debug mode works on alpine: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/18013008390/job/51250509812#step:8:2473 :
```
/usr/include/c++/14.2.0/debug/vector:508:
In function:
constexpr std::debug::vector<_Tp, _Allocator>::reference std::
debug::vector<_Tp, _Allocator>::operator[](size_type) [with _Tp =
CTxOut; _Allocator = std::allocator<CTxOut>; reference = CTxOut&;
size_type = long unsigned int]
Error: attempt to subscrip
...
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3335108541)
Also confirmed that the gcc debug mode works on alpine: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/18013008390/job/51250509812#step:8:2473 :
```
/usr/include/c++/14.2.0/debug/vector:508:
In function:
constexpr std::debug::vector<_Tp, _Allocator>::reference std::
debug::vector<_Tp, _Allocator>::operator[](size_type) [with _Tp =
CTxOut; _Allocator = std::allocator<CTxOut>; reference = CTxOut&;
size_type = long unsigned int]
Error: attempt to subscrip
...
💬 achow101 commented on issue "No way to clear command history in RPC console or reset the console without restarting the node":
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3335117139)
There's already a clear history button in the top right corner of the console window, next to the font size buttons. It works fine for me.
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3335117139)
There's already a clear history button in the top right corner of the console window, next to the font size buttons. It works fine for me.
💬 waketraindev commented on issue "No way to clear command history in RPC console or reset the console without restarting the node":
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3335127373)
> There's already a clear history button in the top right corner of the console window, next to the font size buttons. It works fine for me.
Hello and thank you for the reply. I'm actually talking about the command input history, the one in the input box where you use the arrow keys to browse
Hope that clears it
(https://github.com/bitcoin-core/gui/issues/897#issuecomment-3335127373)
> There's already a clear history button in the top right corner of the console window, next to the font size buttons. It works fine for me.
Hello and thank you for the reply. I'm actually talking about the command input history, the one in the input box where you use the arrow keys to browse
Hope that clears it
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2379831421)
Compiling at commit 2a49082e491810fab48485e0a63e1fadfb09b1b7 currently fails, as the `musig2_`... fields haven't been added to the `SignatureData` struct yet.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2379831421)
Compiling at commit 2a49082e491810fab48485e0a63e1fadfb09b1b7 currently fails, as the `musig2_`... fields haven't been added to the `SignatureData` struct yet.
💬 polespinasa commented on issue "`generateblock` RPC Not Collecting Transaction Fees":
(https://github.com/bitcoin/bitcoin/issues/31684#issuecomment-3335167174)
I think you might want to take a look to #32468.
At the moment it only collects fees when the mempool option is used and not when a set of txs is provided manually, but it's a start :)
(https://github.com/bitcoin/bitcoin/issues/31684#issuecomment-3335167174)
I think you might want to take a look to #32468.
At the moment it only collects fees when the mempool option is used and not when a set of txs is provided manually, but it's a start :)
🤔 maflcko reviewed a pull request: "rpc: require integer verbosity; remove boolean 'verbose'"
(https://github.com/bitcoin/bitcoin/pull/33214#pullrequestreview-3268772493)
not sure where this pull is going, but it could make sense to split out the test changes into a separate pull (replacing the deprecated values with integers and named args), except for one deprecated value for each RPC, so that all code branches remain tests, but most code is using the new way and named args.
(https://github.com/bitcoin/bitcoin/pull/33214#pullrequestreview-3268772493)
not sure where this pull is going, but it could make sense to split out the test changes into a separate pull (replacing the deprecated values with integers and named args), except for one deprecated value for each RPC, so that all code branches remain tests, but most code is using the new way and named args.
💬 maflcko commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2379836497)
this is still moved
(https://github.com/bitcoin/bitcoin/pull/33214#discussion_r2379836497)
this is still moved