💬 maflcko commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553420053)
This also seems unrelated to this pull request? If I increase `-j` sufficiently, I get errors before and after this pull request.
For reference, the error is:
```
./bld-cmake/test/functional/test_runner.py -j 40
Temporary test directory at /tmp/test_runner_₿_🏃_20241219_105517
1/315 - wallet_conflicts.py --legacy-wallet skipped (BDB has not been compiled.)
2/315 - rpc_psbt.py --legacy-wallet skipped (BDB has not been compiled.)
3/315 - wallet_fundrawtransaction.py --legacy-wallet s
...
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553420053)
This also seems unrelated to this pull request? If I increase `-j` sufficiently, I get errors before and after this pull request.
For reference, the error is:
```
./bld-cmake/test/functional/test_runner.py -j 40
Temporary test directory at /tmp/test_runner_₿_🏃_20241219_105517
1/315 - wallet_conflicts.py --legacy-wallet skipped (BDB has not been compiled.)
2/315 - rpc_psbt.py --legacy-wallet skipped (BDB has not been compiled.)
3/315 - wallet_fundrawtransaction.py --legacy-wallet s
...
💬 hodlinator commented on pull request "fuzz: Fix misplaced SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2553435131)
I was worried that `g_rng_temp_path` had somehow stopped working again, but I see you needed to replace it with `m_rng` in your diff to make paths deterministic.
It is a dilemma regarding the hash salts, one would like to exercise many values for them, but still get determinism. What would you think about having the fuzz input affect the random seed?
(https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2553435131)
I was worried that `g_rng_temp_path` had somehow stopped working again, but I see you needed to replace it with `m_rng` in your diff to make paths deterministic.
It is a dilemma regarding the hash salts, one would like to exercise many values for them, but still get determinism. What would you think about having the fuzz input affect the random seed?
📝 l0rinc opened a pull request: "optimization: buffer reads/writes in [undo]blocks"
(https://github.com/bitcoin/bitcoin/pull/31539)
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/31490 (first few commits here, hence a draft) and a precursor to https://github.com/bitcoin/bitcoin/pull/31144.
Currently, obfuscation operations are performed byte-by-byte during serialization. Buffering the reads allows batching these operations (implemented in https://github.com/bitcoin/bitcoin/pull/31144) and improves file access efficiency by reducing fread calls and associated locking overhead. Testing with various buffer si
...
(https://github.com/bitcoin/bitcoin/pull/31539)
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/31490 (first few commits here, hence a draft) and a precursor to https://github.com/bitcoin/bitcoin/pull/31144.
Currently, obfuscation operations are performed byte-by-byte during serialization. Buffering the reads allows batching these operations (implemented in https://github.com/bitcoin/bitcoin/pull/31144) and improves file access efficiency by reducing fread calls and associated locking overhead. Testing with various buffer si
...
💬 Sjors commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2553441321)
@A-Manning thanks, can you squash the commits?
cc @maflcko
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2553441321)
@A-Manning thanks, can you squash the commits?
cc @maflcko
💬 hebasto commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553446026)
> This also seems unrelated to this pull request? If I increase `-j` sufficiently, I get errors before and after this pull request.
That diff causes `test_runner.py` to fail even with the default `-j` (at least on my Ubuntu 24.04).
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553446026)
> This also seems unrelated to this pull request? If I increase `-j` sufficiently, I get errors before and after this pull request.
That diff causes `test_runner.py` to fail even with the default `-j` (at least on my Ubuntu 24.04).
💬 A-Manning commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2553452773)
@Sjors
>thanks, can you squash the commits?
Squashed in ccb41ce0e782470a5e6e9d95a8bd337a1d07adcf
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2553452773)
@Sjors
>thanks, can you squash the commits?
Squashed in ccb41ce0e782470a5e6e9d95a8bd337a1d07adcf
💬 Sjors commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891647010)
nit: wrong indentation and unnecessary blank line
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891647010)
nit: wrong indentation and unnecessary blank line
👍 hodlinator approved a pull request: "Remove unused variable assignment"
(https://github.com/bitcoin/bitcoin/pull/31497#pullrequestreview-2514136983)
crACK b9766c9977e58a9ebc358d9879576376e76a72b1
This dead store was a leftover from the introduction of CoinGrinder in 6cc9a46cd0f4ed80d4523bbef1508142e0c80d27 / #27877.
Checked that there are no shenanigans with the variable being captured into a lambda further up which is then called further down.
(https://github.com/bitcoin/bitcoin/pull/31497#pullrequestreview-2514136983)
crACK b9766c9977e58a9ebc358d9879576376e76a72b1
This dead store was a leftover from the introduction of CoinGrinder in 6cc9a46cd0f4ed80d4523bbef1508142e0c80d27 / #27877.
Checked that there are no shenanigans with the variable being captured into a lambda further up which is then called further down.
💬 Sjors commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891651120)
I don't understand why this was duplicated in the original version. @kallewoof do you remember?
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891651120)
I don't understand why this was duplicated in the original version. @kallewoof do you remember?
💬 Sjors commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891655005)
Maybe also add a `check_getmininginfo` helper?
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891655005)
Maybe also add a `check_getmininginfo` helper?
💬 maflcko commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553487975)
> > This also seems unrelated to this pull request? If I increase `-j` sufficiently, I get errors before and after this pull request.
>
> That diff causes `test_runner.py` to fail even with the default `-j` (at least on my Ubuntu 24.04). And I assume that the CI will fail as well.
You supplied `-j40` in your command, which is not the default. As for the patch, `-maxconnections` controls the number of inbound connections (assuming that it is above the number of minimum outbound connections)
...
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553487975)
> > This also seems unrelated to this pull request? If I increase `-j` sufficiently, I get errors before and after this pull request.
>
> That diff causes `test_runner.py` to fail even with the default `-j` (at least on my Ubuntu 24.04). And I assume that the CI will fail as well.
You supplied `-j40` in your command, which is not the default. As for the patch, `-maxconnections` controls the number of inbound connections (assuming that it is above the number of minimum outbound connections)
...
💬 maflcko commented on pull request "fuzz: Fix misplaced SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2553495150)
> It is a dilemma regarding the hash salts, one would like to exercise many values for them, but still get determinism. What would you think about having the fuzz input affect the random seed?
Certainly, this is possible and can be done on a case-by-case basis for fuzz targets where it would be useful.
(https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2553495150)
> It is a dilemma regarding the hash salts, one would like to exercise many values for them, but still get determinism. What would you think about having the fuzz input affect the random seed?
Certainly, this is possible and can be done on a case-by-case basis for fuzz targets where it would be useful.
💬 hebasto commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553497647)
> You supplied `-j40` in your command, which is not the default.
By "default", I mean not providing `-j` at all. This is another run, not the first one with `-j 40`. I apologies for any confusion.
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553497647)
> You supplied `-j40` in your command, which is not the default.
By "default", I mean not providing `-j` at all. This is another run, not the first one with `-j 40`. I apologies for any confusion.
💬 hebasto commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553501435)
> So the failure is expected and my recommendation would be to set the value to the max inbound count (maybe double or triple that).
Do we have a named constant for that in the test framework?
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553501435)
> So the failure is expected and my recommendation would be to set the value to the max inbound count (maybe double or triple that).
Do we have a named constant for that in the test framework?
💬 maflcko commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553506832)
No, the number of inbound connections depends on the test at runtime, depending of how many inbound connections are created.
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553506832)
No, the number of inbound connections depends on the test at runtime, depending of how many inbound connections are created.
💬 hebasto commented on pull request "Do not print "Reducing -maxconnections..." warning in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553646587)
> the goal should be to not have any test-only hooks
Reworked. Only python code is modified now.
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553646587)
> the goal should be to not have any test-only hooks
Reworked. Only python code is modified now.
💬 A-Manning commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891837715)
Added in edc34044362628f2994cb5fa5de74ca49fce0fd1
(https://github.com/bitcoin/bitcoin/pull/31531#discussion_r1891837715)
Added in edc34044362628f2994cb5fa5de74ca49fce0fd1
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1891893833)
This test and the commit 6d43aad742c7ea28303cf2799528188938e7ce32 that introduced it actually have many issues:
* `std::vector<bool>::data` never returned `void*`, only `void`, see https://godbolt.org/z/ePqGxjovv
* The reason why `std::vector<bool>` is not spannable is because the iterator is not contiguous, not due to the type of the data pointer
* `std::span` is more correct in that it does all the checks, so my recommendation would be to just remove the test and all related code, as expl
...
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1891893833)
This test and the commit 6d43aad742c7ea28303cf2799528188938e7ce32 that introduced it actually have many issues:
* `std::vector<bool>::data` never returned `void*`, only `void`, see https://godbolt.org/z/ePqGxjovv
* The reason why `std::vector<bool>` is not spannable is because the iterator is not contiguous, not due to the type of the data pointer
* `std::span` is more correct in that it does all the checks, so my recommendation would be to just remove the test and all related code, as expl
...
💬 hugohn commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2553746288)
Hey guys, we've successfully integrated this into Nunchuk, so you should be able to test this out with a real UI/UX very soon.
MuSig2 key path spend: https://mempool.space/tx/69c75aa798e03dbe782c9a11eed316440fa2a4cb9c4645af2f5d8d566c04207b?mode=details
MuSig2 script path spend: https://mempool.space/tx/73f63684994477924105966e646427f7fc802352d9ba9d1baebf05b1f3dc3fab?mode=details
Sample descriptors:
`tr(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2553746288)
Hey guys, we've successfully integrated this into Nunchuk, so you should be able to test this out with a real UI/UX very soon.
MuSig2 key path spend: https://mempool.space/tx/69c75aa798e03dbe782c9a11eed316440fa2a4cb9c4645af2f5d8d566c04207b?mode=details
MuSig2 script path spend: https://mempool.space/tx/73f63684994477924105966e646427f7fc802352d9ba9d1baebf05b1f3dc3fab?mode=details
Sample descriptors:
`tr(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G
...
💬 maflcko commented on pull request "qa: Ignore "Reducing -maxconnections..." warning in stderr":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553780090)
It is nice to see code changes, but generally for review it would be good to explain the code changes and provide useful context and alternatives.
Specifically, it would be good to explain why you didn't just set maxconnections appropriately.
Silently ignoring the warning seems like a foot-gun to hit later on, when more connections than that are created and the test fails again on netbsd (or whatever), in which case someone has to create another pull.
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553780090)
It is nice to see code changes, but generally for review it would be good to explain the code changes and provide useful context and alternatives.
Specifically, it would be good to explain why you didn't just set maxconnections appropriately.
Silently ignoring the warning seems like a foot-gun to hit later on, when more connections than that are created and the test fails again on netbsd (or whatever), in which case someone has to create another pull.