👍 ryanofsky approved a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2058984290)
Code review ACK 35b5b0ff9e043c1e26a3e29fa3d9d914a0a86ad1. I confirmed this change is only adding std::moves, and also tried to look for cases where an object was being used after a move, since that seems like the most likely type of bug this PR could introduce. I didn't check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2058984290)
Code review ACK 35b5b0ff9e043c1e26a3e29fa3d9d914a0a86ad1. I confirmed this change is only adding std::moves, and also tried to look for cases where an object was being used after a move, since that seems like the most likely type of bug this PR could introduce. I didn't check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
📝 furszy opened a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118)
Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.
The `connect_nodes` function in the test framework relies on a stable number of peer
connections to verify the new connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent `wait_until` checks to stall i
...
(https://github.com/bitcoin/bitcoin/pull/30118)
Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.
The `connect_nodes` function in the test framework relies on a stable number of peer
connections to verify the new connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent `wait_until` checks to stall i
...
✅ hebasto closed a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/30117)
(https://github.com/bitcoin/bitcoin/pull/30117)
📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/30117)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are a
...
(https://github.com/bitcoin/bitcoin/pull/30117)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are a
...
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602301949)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" 7c047d30cb0eadc8a424cb01de2fcd0978e22206: What is the step 2? This comment seems a little confuse because I couldn't find it as I see for Step 1.
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602301949)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" 7c047d30cb0eadc8a424cb01de2fcd0978e22206: What is the step 2? This comment seems a little confuse because I couldn't find it as I see for Step 1.
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602304001)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" https://github.com/bitcoin/bitcoin/commit/7c047d30cb0eadc8a424cb01de2fcd0978e22206: Perhaps adding a log when removing from set?
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1602304001)
In "p2p: Functions to add/remove wtxids to tx reconciliation sets" https://github.com/bitcoin/bitcoin/commit/7c047d30cb0eadc8a424cb01de2fcd0978e22206: Perhaps adding a log when removing from set?
💬 kevkevinpal commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1602311815)
Thanks for the review!
I think the timeout should be fine there is even a comment pointing out that we check the work twice because we want to fail fast
https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5655-L5657
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1602311815)
Thanks for the review!
I think the timeout should be fine there is even a comment pointing out that we check the work twice because we want to fail fast
https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5655-L5657
💬 1ma commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2113592794)
Can reproduce. Been running a signet network whose miner stalled a few dozens of blocks after the block 10080 difficulty adjustment.
The miner is ran as a systemd service like this, and the nbits parameter was determined by using the calibrating utility set to 600s. Despite this, before block 10080 the miner used to mine a block every 2.5 min instead of 10. I don't quite understand how it works, honestly.
```shell
miner --cli="bitcoin-cli" generate --address $rewards_addy --grind-cmd="bit
...
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2113592794)
Can reproduce. Been running a signet network whose miner stalled a few dozens of blocks after the block 10080 difficulty adjustment.
The miner is ran as a systemd service like this, and the nbits parameter was determined by using the calibrating utility set to 600s. Despite this, before block 10080 the miner used to mine a block every 2.5 min instead of 10. I don't quite understand how it works, honestly.
```shell
miner --cli="bitcoin-cli" generate --address $rewards_addy --grind-cmd="bit
...
✅ fanquake closed an issue: "Compilation failure with `-O0` + `-fsanitize=address` due to inline asm"
(https://github.com/bitcoin/bitcoin/issues/29801)
(https://github.com/bitcoin/bitcoin/issues/29801)
🚀 fanquake merged a pull request: "crypto: disable asan for sha256_sse4 with clang and -O0"
(https://github.com/bitcoin/bitcoin/pull/30097)
(https://github.com/bitcoin/bitcoin/pull/30097)
💬 fanquake commented on pull request "crypto: disable asan for sha256_sse4 with clang and -O0":
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2113705333)
Backported to 27.x in #30092.
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2113705333)
Backported to 27.x in #30092.
👍 tdb3 approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2059283785)
re ACK for cbc6c440e3811d342fa570713702900b3e3e75b9
Great work. Performed brief code review. Re-ran the tests described in https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1922293722 again (actions 1 through 7, with the caveat that v27.0 was used as the baseline for comparison rather v26.0, and regtest was used). Everything worked as expected. Also ran all functional tests (passed).
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2059283785)
re ACK for cbc6c440e3811d342fa570713702900b3e3e75b9
Great work. Performed brief code review. Re-ran the tests described in https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1922293722 again (actions 1 through 7, with the caveat that v27.0 was used as the baseline for comparison rather v26.0, and regtest was used). Everything worked as expected. Also ran all functional tests (passed).
💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2113723824)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2113723824)
Rebased.
💬 theuni commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-2113764897)
@ryanofsky Sorry for not getting to this before merge. Will give it a look tomorrow.
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-2113764897)
@ryanofsky Sorry for not getting to this before merge. Will give it a look tomorrow.
💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2113778062)
I didn't check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
Indeed, I believe c-i should be doing that check. But just in case, from the PR description:
> I ran these changes through clang-tidy with performance-move-const-arg and bugprone-use-after-move and no bugs were detected (though that's obviously not to say it can be trusted 100%).
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2113778062)
I didn't check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
Indeed, I believe c-i should be doing that check. But just in case, from the PR description:
> I ran these changes through clang-tidy with performance-move-const-arg and bugprone-use-after-move and no bugs were detected (though that's obviously not to say it can be trusted 100%).
💬 edilmedeiros commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2113923344)
Thanks for reporting.
About the 2m30s interval, see #30091.
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2113923344)
Thanks for reporting.
About the 2m30s interval, see #30091.
📝 fanquake opened a pull request: "Update libsecp256k1 subtree to current master"
(https://github.com/bitcoin/bitcoin/pull/30120)
This includes changes from the 0.5.0 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.5.0
> New function secp256k1_ec_pubkey_sort that sorts public keys using lexicographic (of compressed serialization) order.
> The implementation of the point multiplication algorithm used for signing and public key generation was changed, resulting in improved performance for those operations.
> The related configure option --ecmult-gen-precision was replaced with --ecmult-gen-kb (EC
...
(https://github.com/bitcoin/bitcoin/pull/30120)
This includes changes from the 0.5.0 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.5.0
> New function secp256k1_ec_pubkey_sort that sorts public keys using lexicographic (of compressed serialization) order.
> The implementation of the point multiplication algorithm used for signing and public key generation was changed, resulting in improved performance for those operations.
> The related configure option --ecmult-gen-precision was replaced with --ecmult-gen-kb (EC
...
💬 fanquake commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/30120#issuecomment-2113932737)
cc @real-or-random @jonasnick
(https://github.com/bitcoin/bitcoin/pull/30120#issuecomment-2113932737)
cc @real-or-random @jonasnick
🚀 fanquake merged a pull request: "p2p: detect addnode cjdns peers in GetAddedNodeInfo()"
(https://github.com/bitcoin/bitcoin/pull/30085)
(https://github.com/bitcoin/bitcoin/pull/30085)
🤔 edilmedeiros reviewed a pull request: "signet: fixing mining for OP_TRUE challenge"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2059510296)
Concept ACK
Code looks good, I'm still to test it.
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2059510296)
Concept ACK
Code looks good, I'm still to test it.