💬 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.
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724120384)
> > 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.
Have incorporated your suggestion
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724120384)
> > 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.
Have incorporated your suggestion
...
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995241573)
At some point we might want to have something like `-salvage` for corrupted sqlite wallets, but makes sense to remove it for now.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995241573)
At some point we might want to have something like `-salvage` for corrupted sqlite wallets, but makes sense to remove it for now.
👍 brunoerg approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2684995699)
reACK 32a522f2d825957f0c85d7b4ea9185a053b018e3
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2684995699)
reACK 32a522f2d825957f0c85d7b4ea9185a053b018e3
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995257444)
missing `return false;`?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995257444)
missing `return false;`?
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1995271241)
Thanks, yes it often takes a little while to sync up so I think 0.1 would be slightly better than the default 0.05. Will do if I re-touch.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1995271241)
Thanks, yes it often takes a little while to sync up so I think 0.1 would be slightly better than the default 0.05. Will do if I re-touch.