✅ fanquake closed a pull request: "docs: Enhance TODO section in README.md with detailed explanations"
(https://github.com/bitcoin/bitcoin/pull/32048)
(https://github.com/bitcoin/bitcoin/pull/32048)
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317)
I omit the `#` in cases like this, where it isn't really important to link them here, so that GitHub does not add a notification in that PR every time this branch is (re)pushed.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317)
I omit the `#` in cases like this, where it isn't really important to link them here, so that GitHub does not add a notification in that PR every time this branch is (re)pushed.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992477649)
Yes, here it seems cleaner, fitting the use case and requiring less code to do the same thing. I appreciate your indulgence here.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992477649)
Yes, here it seems cleaner, fitting the use case and requiring less code to do the same thing. I appreciate your indulgence here.
📝 theStack opened a pull request: "test: avoid treating hash results as integers (part 1)"
(https://github.com/bitcoin/bitcoin/pull/32050)
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and it almost never makes sense to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC
...
(https://github.com/bitcoin/bitcoin/pull/32050)
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and it almost never makes sense to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC
...
💬 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.