💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992480335)
Your review comment provides that context (thanks). I wouldn't usually add that to the commit message for the same reason as https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317. In this case, however, where the PR discussion is locked, maybe it wouldn't add notifications there. Not sure.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992480335)
Your review comment provides that context (thanks). I wouldn't usually add that to the commit message for the same reason as https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317. In this case, however, where the PR discussion is locked, maybe it wouldn't add notifications there. Not sure.
💬 jonatack commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2719457484)
Concept ACK, will begin nuts and bolts review.
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2719457484)
Concept ACK, will begin nuts and bolts review.
✅ fanquake closed an issue: "test: 32-bit Clang `ipc_test` failure at `-O0`"
(https://github.com/bitcoin/bitcoin/issues/31772)
(https://github.com/bitcoin/bitcoin/issues/31772)
🚀 fanquake merged a pull request: "depends: patch around PlacementNew issue in capnp"
(https://github.com/bitcoin/bitcoin/pull/31998)
(https://github.com/bitcoin/bitcoin/pull/31998)
👍 fanquake approved a pull request: "ci: Revert "Temporary workaround for old CCACHE_DIR cirrus env""
(https://github.com/bitcoin/bitcoin/pull/32044#pullrequestreview-2680189403)
ACK fa21597064b8599b04461b8d3553e802e47d063e
(https://github.com/bitcoin/bitcoin/pull/32044#pullrequestreview-2680189403)
ACK fa21597064b8599b04461b8d3553e802e47d063e
🚀 fanquake merged a pull request: "ci: Revert "Temporary workaround for old CCACHE_DIR cirrus env""
(https://github.com/bitcoin/bitcoin/pull/32044)
(https://github.com/bitcoin/bitcoin/pull/32044)
💬 davidgumberg commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719475327)
ACK https://github.com/bitcoin/bitcoin/commit/47e2fa86dc5433852fd9e5050a23de2accfdca8d
I'm ~0 on whether or not the release candidate should be held up by the example `bitcoin.conf`, but everything else looks good to me.
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719475327)
ACK https://github.com/bitcoin/bitcoin/commit/47e2fa86dc5433852fd9e5050a23de2accfdca8d
I'm ~0 on whether or not the release candidate should be held up by the example `bitcoin.conf`, but everything else looks good to me.
💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2719581088)
Addressing the review from [comment](https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
- The coverage now includes functional tests
- Removed coverage from external deps (crc32,leveldb,minisketch,secp256k1) and `src/test`.
- Now uses `-DCMAKE_BUILD_TYPE=Debug`
- Notes are now in Blockquotes
- Incuding the functional tests has increased the mismatch from 2 to 61 .
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2719581088)
Addressing the review from [comment](https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
- The coverage now includes functional tests
- Removed coverage from external deps (crc32,leveldb,minisketch,secp256k1) and `src/test`.
- Now uses `-DCMAKE_BUILD_TYPE=Debug`
- Notes are now in Blockquotes
- Incuding the functional tests has increased the mismatch from 2 to 61 .
📝 jonatack opened a pull request: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051)
When an addnode peer is disconnected by our IBD headers/blocks download timeout or stalling logic, ThreadOpenAddedConnections attempts to immediately reconnect it.
During IBD in a low-bandwidth environment, I see these disconnections/reconnections frequently with high-quality addnode peers.
This pull attempts to avoid unnecessary disconnections where it may make sense to do so. For now, it is an initial quick draft.
(https://github.com/bitcoin/bitcoin/pull/32051)
When an addnode peer is disconnected by our IBD headers/blocks download timeout or stalling logic, ThreadOpenAddedConnections attempts to immediately reconnect it.
During IBD in a low-bandwidth environment, I see these disconnections/reconnections frequently with high-quality addnode peers.
This pull attempts to avoid unnecessary disconnections where it may make sense to do so. For now, it is an initial quick draft.
💬 fanquake commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2719746790)
```bash
[23:32:25.656] /ci_container_base/src/rpc/net.cpp:310:125: error: invalid operands to binary expression ('const char[198]' and 'const char[196]')
[23:32:25.656] "Nodes added using addnode (or -connect) are protected from disconnection due to DoS or IBD header/block\n" +
[23:32:25.656] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
[23:32:25.656] 1 error generated.
```
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2719746790)
```bash
[23:32:25.656] /ci_container_base/src/rpc/net.cpp:310:125: error: invalid operands to binary expression ('const char[198]' and 'const char[196]')
[23:32:25.656] "Nodes added using addnode (or -connect) are protected from disconnection due to DoS or IBD header/block\n" +
[23:32:25.656] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
[23:32:25.656] 1 error generated.
```
💬 fanquake commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719771413)
There's likely going to be more rcs than usual, so just getting this out for testing with the incomplete example .conf is fine.
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719771413)
There's likely going to be more rcs than usual, so just getting this out for testing with the incomplete example .conf is fine.
🚀 fanquake merged a pull request: "[29.x] bump to v29.0rc1"
(https://github.com/bitcoin/bitcoin/pull/32046)
(https://github.com/bitcoin/bitcoin/pull/32046)
💬 Prabhat1308 commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2719819873)
Are the files mentioned in `files_to_delete` and `files_to_perturb` just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2719819873)
Are the files mentioned in `files_to_delete` and `files_to_perturb` just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
🤔 glozow reviewed a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2680493978)
ACK a24419f8bed5e1145ce171dbbdad957750585471
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2680493978)
ACK a24419f8bed5e1145ce171dbbdad957750585471
🚀 fanquake merged a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901)
(https://github.com/bitcoin/bitcoin/pull/31901)
🤔 furszy reviewed a pull request: "removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2680558849)
Please try to avoid introducing and reverting changes within the same PR. The first two commits should be squashed. Aside from that, I left you a comment suggesting a possible alternative implementation.
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2680558849)
Please try to avoid introducing and reverting changes within the same PR. The first two commits should be squashed. Aside from that, I left you a comment suggesting a possible alternative implementation.
💬 furszy commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1992789597)
What about returning `util::Result<void>` instead and return the error message here? (see how we do it in other places).
Also, if you're going down this route, you should check any other usages of this function, as it would no longer be throwing an exception.
And you would also have to return an `util::Result<ScriptPubKeyMan*>` in `AddWalletDescriptor` too. Which I think that is cleaner than catching the specific exception.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1992789597)
What about returning `util::Result<void>` instead and return the error message here? (see how we do it in other places).
Also, if you're going down this route, you should check any other usages of this function, as it would no longer be throwing an exception.
And you would also have to return an `util::Result<ScriptPubKeyMan*>` in `AddWalletDescriptor` too. Which I think that is cleaner than catching the specific exception.
💬 davidrobinsonau commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992796261)
Is there a use case for someone to pass a timestamp prior to January 1, 1970? Which would be a negative number, causing bitcoind not to rescan when it was expected to.
Alternatively, the code could be updated to only look for "-1", meaning anything below that could still be a full rescan.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992796261)
Is there a use case for someone to pass a timestamp prior to January 1, 1970? Which would be a negative number, causing bitcoind not to rescan when it was expected to.
Alternatively, the code could be updated to only look for "-1", meaning anything below that could still be a full rescan.
⚠️ fanquake opened an issue: "v29.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
⚠️ fanquake pinned an issue: "v29.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...