💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994984394)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994984394)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1994997845)
Thanks and sorry for missing that bash/sh are obviously not used on Windows. I missed the `cmd` section when looking at the link.
Still, it feels a bit uncomfortable that we are using `cmd` on Windows in the CI, which will by default ignore intermediate exit codes.
Given that neither the pull request author, nor any reviewer caught this before someone found it by testing, doesn't increase confidence either.
Thus, I think we should somehow rule out this class of bug, or at least documen
...
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1994997845)
Thanks and sorry for missing that bash/sh are obviously not used on Windows. I missed the `cmd` section when looking at the link.
Still, it feels a bit uncomfortable that we are using `cmd` on Windows in the CI, which will by default ignore intermediate exit codes.
Given that neither the pull request author, nor any reviewer caught this before someone found it by testing, doesn't increase confidence either.
Thus, I think we should somehow rule out this class of bug, or at least documen
...
💬 maflcko commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193)
> Would it make sense to also drop `ENABLE_HARDENING`? Currently set `OFF` in the `clang-tidy` ci job, but I [flipped it `ON`](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)
IIRC it was added in fab24f8c3540b6f1a128cb9d6812df6678472b8d, because I copy-pasted it from another CI task and forgot to remove it. I guess
...
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193)
> Would it make sense to also drop `ENABLE_HARDENING`? Currently set `OFF` in the `clang-tidy` ci job, but I [flipped it `ON`](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)
IIRC it was added in fab24f8c3540b6f1a128cb9d6812df6678472b8d, because I copy-pasted it from another CI task and forgot to remove it. I guess
...
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723935664)
No strong opinion, but it could make sense to reword the pull description. Otherwise, it is less clear what all the effects of this will be. Something like:
> This adds support for macOS to `ResetCoverageCounters`. `ResetCoverageCounters` is used by the unit tests in `g_rng_temp_path_init` to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.
> Refer to ... Readme on how to test this for deterministic unit
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2723935664)
No strong opinion, but it could make sense to reword the pull description. Otherwise, it is less clear what all the effects of this will be. Something like:
> This adds support for macOS to `ResetCoverageCounters`. `ResetCoverageCounters` is used by the unit tests in `g_rng_temp_path_init` to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.
> Refer to ... Readme on how to test this for deterministic unit
...
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995079532)
Rebased the cod and changed the PR description based on feedback , thank you for the review!
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995079532)
Rebased the cod and changed the PR description based on feedback , thank you for the review!
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995082229)
Good point, I have removed the check in the macro and rebased my commits. Thank you for the review.
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995082229)
Good point, I have removed the check in the macro and rebased my commits. Thank you for the review.
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995085087)
Thank you for the suggestion, have found a way to fix the code so that the linker does not break and the functionality is the same. Thank you for the review
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995085087)
Thank you for the suggestion, have found a way to fix the code so that the linker does not break and the functionality is the same. Thank you for the review
💬 maflcko commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995088556)
This change makes the assert a no-op and turns it undefined behavior.
Previously, the `block.data` presence was asserted before the `for`-loop. Now, the for loop will run into UB when `block.data` is null. (It is true that the for-loop body still checks for the presence, but once UB has happened, it is not possible to recover from it)
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995088556)
This change makes the assert a no-op and turns it undefined behavior.
Previously, the `block.data` presence was asserted before the `for`-loop. Now, the for loop will run into UB when `block.data` is null. (It is true that the for-loop body still checks for the presence, but once UB has happened, it is not possible to recover from it)
💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2723975436)
Rebased after #31649.
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2723975436)
Rebased after #31649.
📝 vasild opened a pull request: "i2p: make a time gap between creating transient sessions and using them"
(https://github.com/bitcoin/bitcoin/pull/32065)
Connecting to an I2P peer consists of creating a session (the
`SESSION CREATE` command) and then connecting to the peer using that
session (`STREAM CONNECT ID=session_id ...`).
This change is only relevant for transient sessions because when a
persistent session is used it is created once and used for all
connections.
Before this change Bitcoin Core would create the session and use it in
quick succession. That is, the `SESSION CREATE` command would be
immediately followed by `STREAM
...
(https://github.com/bitcoin/bitcoin/pull/32065)
Connecting to an I2P peer consists of creating a session (the
`SESSION CREATE` command) and then connecting to the peer using that
session (`STREAM CONNECT ID=session_id ...`).
This change is only relevant for transient sessions because when a
persistent session is used it is created once and used for all
connections.
Before this change Bitcoin Core would create the session and use it in
quick succession. That is, the `SESSION CREATE` command would be
immediately followed by `STREAM
...
💬 maflcko commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1995129869)
This whole test seems a bit confusing, especially with this comment. The goal of the test is to check that a wallet doesn't load cross-chain silently. It seems confusing to mention testnet3 and testnet4 here, but no other networks. Either, all test networks are tested, or only a single one. I'd argue that a single one should be sufficient.
Thus, a clean revert of `git show 74a04f9e7ad6a16988149cc3438b9ce13c91cdb9 ./test/functional/wallet_crosschain.py` seems more appropriate. Then, a follow-u
...
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1995129869)
This whole test seems a bit confusing, especially with this comment. The goal of the test is to check that a wallet doesn't load cross-chain silently. It seems confusing to mention testnet3 and testnet4 here, but no other networks. Either, all test networks are tested, or only a single one. I'd argue that a single one should be sufficient.
Thus, a clean revert of `git show 74a04f9e7ad6a16988149cc3438b9ce13c91cdb9 ./test/functional/wallet_crosschain.py` seems more appropriate. Then, a follow-u
...
💬 davidrobinsonau commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995133082)
This code is getting complex for a simple change. @maflcko, you stated above that instead of having another field, "rescan", we should simply use a value for the timestamp field to advise if a rescan is needed. This has been done, but sadly, the function "static int64_t GetImportTimestamp(const UniValue& data, int64_t now)" only returns an int64_t, and we need to work with that returned value, AFAIK, we can't use null as its not a pointer. So that leaves a number, which is currently -1.
The
...
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995133082)
This code is getting complex for a simple change. @maflcko, you stated above that instead of having another field, "rescan", we should simply use a value for the timestamp field to advise if a rescan is needed. This has been done, but sadly, the function "static int64_t GetImportTimestamp(const UniValue& data, int64_t now)" only returns an int64_t, and we need to work with that returned value, AFAIK, we can't use null as its not a pointer. So that leaves a number, which is currently -1.
The
...
⚠️ maflcko opened an issue: "intermittent issue in wallet_reorgsrestore.py in test_reorg_handling_during_unclean_shutdown: FailedToStartError: [node 0] bitcoind exited with status 1 during initialization. Error: Cannot obtain a lock on directory"
(https://github.com/bitcoin/bitcoin/issues/32066)
https://cirrus-ci.com/task/4809537674280960?logs=ci#L2287:
```
[04:43:25.506] test 2025-03-14T08:43:24.312000Z TestFramework.utils (DEBUG): Deleting leftover cookie file
[04:43:25.506] test 2025-03-14T08:43:24.315000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
[04:43:25.506] test 2025-03-14T08:43:24.576000Z TestFramework (ERROR): Unexpected exception caught during testing
[04:43:25.506] Traceback (most recent call last):
...
(https://github.com/bitcoin/bitcoin/issues/32066)
https://cirrus-ci.com/task/4809537674280960?logs=ci#L2287:
```
[04:43:25.506] test 2025-03-14T08:43:24.312000Z TestFramework.utils (DEBUG): Deleting leftover cookie file
[04:43:25.506] test 2025-03-14T08:43:24.315000Z TestFramework.node0 (DEBUG): bitcoind started, waiting for RPC to come up
[04:43:25.506] test 2025-03-14T08:43:24.576000Z TestFramework (ERROR): Unexpected exception caught during testing
[04:43:25.506] Traceback (most recent call last):
...
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724051527)
> See [Readme](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md) on how to test this for deterministic unit test
I guess this can be used to test both (unit and fuzz tests)? If yes, it could make sense to recommend a unit test and fuzz test, each. For example `util_string_tests` and `addition_overflow` (haven't tested either)? Before, the coverage should be non-deterministic, and after this change, it should be deterministic.
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724051527)
> See [Readme](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md) on how to test this for deterministic unit test
I guess this can be used to test both (unit and fuzz tests)? If yes, it could make sense to recommend a unit test and fuzz test, each. For example `util_string_tests` and `addition_overflow` (haven't tested either)? Before, the coverage should be non-deterministic, and after this change, it should be deterministic.
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2724057711)
Rebased after #31974 (trivial conflict with 3b33fd9424a2b4edbdc1745339e3541ab4b90af5).
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2724057711)
Rebased after #31974 (trivial conflict with 3b33fd9424a2b4edbdc1745339e3541ab4b90af5).
💬 Sjors commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2724058966)
Posted on the mailinglist: https://gnusha.org/pi/bitcoindev/9FAA7EEC-BD22-491E-B21B-732AEA15F556@sprovoost.nl/T/#u
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2724058966)
Posted on the mailinglist: https://gnusha.org/pi/bitcoindev/9FAA7EEC-BD22-491E-B21B-732AEA15F556@sprovoost.nl/T/#u
💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995170672)
> This code is getting complex for a simple change. @maflcko, you stated above that instead of having another field, "rescan", we should simply use a value for the timestamp field to advise if a rescan is needed. This has been done, but sadly, the function "static int64_t GetImportTimestamp(const UniValue& data, int64_t now)" only returns an int64_t, and we need to work with that returned value, AFAIK, we can't use null as its not a pointer. So that leaves a number, which is currently -1.
In
...
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995170672)
> This code is getting complex for a simple change. @maflcko, you stated above that instead of having another field, "rescan", we should simply use a value for the timestamp field to advise if a rescan is needed. This has been done, but sadly, the function "static int64_t GetImportTimestamp(const UniValue& data, int64_t now)" only returns an int64_t, and we need to work with that returned value, AFAIK, we can't use null as its not a pointer. So that leaves a number, which is currently -1.
In
...
👍 TheCharlatan approved a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2684875549)
Re-ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
Just addressed nits; added some more docs and tests for `IsActiveAfter`.
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2684875549)
Re-ACK e3014017bacff42d8d69f3061ce1ee621aaa450a
Just addressed nits; added some more docs and tests for `IsActiveAfter`.
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2724090784)
Hoorway, #31961 landed.
re-ACK 32a522f2d825957f0c85d7b4ea9185a053b018e3
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2724090784)
Hoorway, #31961 landed.
re-ACK 32a522f2d825957f0c85d7b4ea9185a053b018e3
💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1995196173)
I made a note to split this off, unless someone else gets to it first.
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1995196173)
I made a note to split this off, unless someone else gets to it first.