ðŽ stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2398531222)
Done.
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2398531222)
Done.
ðŽ maflcko commented on pull request "Update libmultiprocess subtree to support reduced logging":
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3360857978)
I think the nix config is not using an instrumented libcpp, which could be a reason for the difference? C.f. `USE_INSTRUMENTED_LIBCPP="Thread"`.
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3360857978)
I think the nix config is not using an instrumented libcpp, which could be a reason for the difference? C.f. `USE_INSTRUMENTED_LIBCPP="Thread"`.
ð fanquake merged a pull request: "Mempool: Do not enforce TRUC checks on reorg"
(https://github.com/bitcoin/bitcoin/pull/33504)
(https://github.com/bitcoin/bitcoin/pull/33504)
ðŽ luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2398673387)
At the time, you missed maxfeerate, but I see that has since been added
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2398673387)
At the time, you missed maxfeerate, but I see that has since been added
ðŽ luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3361035802)
>We don't want to send invalid txs or txs that we would not broadcast,. We want to validates the txs before sending it to a peer.
Maybe it should just do validation then, and can be followed up by the raw send?
>Additionally, peer_id argument could take an array of ids and send the tx to multiple peers. (this is not implemented, but shouldn't be difficult to)
Sounds like a job for batching.
(But maybe it's enough complexity to be worth a simplified RPC like this)
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3361035802)
>We don't want to send invalid txs or txs that we would not broadcast,. We want to validates the txs before sending it to a peer.
Maybe it should just do validation then, and can be followed up by the raw send?
>Additionally, peer_id argument could take an array of ids and send the tx to multiple peers. (this is not implemented, but shouldn't be difficult to)
Sounds like a job for batching.
(But maybe it's enough complexity to be worth a simplified RPC like this)
ðĪ Eunovo reviewed a pull request: "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3294444435)
Concept ACK https://github.com/bitcoin/bitcoin/pull/33184/commits/1ffdf5253388af3cf274efd723b7129d9639da8c
The test is much simpler and the untested path has been eliminated, but I left the comment about the `OP_RETURN` test below.
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3294444435)
Concept ACK https://github.com/bitcoin/bitcoin/pull/33184/commits/1ffdf5253388af3cf274efd723b7129d9639da8c
The test is much simpler and the untested path has been eliminated, but I left the comment about the `OP_RETURN` test below.
ðŽ Eunovo commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2398710391)
https://github.com/bitcoin/bitcoin/pull/33184/commits/1ffdf5253388af3cf274efd723b7129d9639da8c
This is basically testing that two results of `getblockstats` match each other. It doesn't actually determine the `expected_tip_stats` in the test and then compare against the actual result of `getblockstats`.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2398710391)
https://github.com/bitcoin/bitcoin/pull/33184/commits/1ffdf5253388af3cf274efd723b7129d9639da8c
This is basically testing that two results of `getblockstats` match each other. It doesn't actually determine the `expected_tip_stats` in the test and then compare against the actual result of `getblockstats`.
ðĪ ismaelsadeeq reviewed a pull request: "contrib: fix macOS deployment with no translations"
(https://github.com/bitcoin/bitcoin/pull/33482#pullrequestreview-3294494300)
Thanks for attempting to contribute, welcome ðŠī
The config comments also mentioned that adding translation file is optional.
```
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/b/c/macdeploy (master) [2]> ./macdeployqtplus --help
usage: macdeployqtplus [-h] [-verbose [VERBOSE]] [-no-plugins] [-no-strip] [-translations-dir path] [-zip zip] app-bundle
Improved version of macdeployqt. Outputs a ready-to-deploy app in a folder "dist" and optionally wraps it in a .zip file. Note, that the "dist" fol
...
(https://github.com/bitcoin/bitcoin/pull/33482#pullrequestreview-3294494300)
Thanks for attempting to contribute, welcome ðŠī
The config comments also mentioned that adding translation file is optional.
```
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/b/c/macdeploy (master) [2]> ./macdeployqtplus --help
usage: macdeployqtplus [-h] [-verbose [VERBOSE]] [-no-plugins] [-no-strip] [-translations-dir path] [-zip zip] app-bundle
Improved version of macdeployqt. Outputs a ready-to-deploy app in a folder "dist" and optionally wraps it in a .zip file. Note, that the "dist" fol
...
ðĪ ismaelsadeeq reviewed a pull request: "contrib: fix macOS deployment with no translations"
(https://github.com/bitcoin/bitcoin/pull/33482#pullrequestreview-3294495650)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33482#pullrequestreview-3294495650)
Concept ACK
ðŽ fanquake commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#issuecomment-3361147273)
Backported to 30.x in #33473.
(https://github.com/bitcoin/bitcoin/pull/33504#issuecomment-3361147273)
Backported to 30.x in #33473.
ðŽ fanquake commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3361148614)
Backported to 30.x in #33473.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3361148614)
Backported to 30.x in #33473.
ðĪ vasild reviewed a pull request: "Improve LastCommonAncestor performance + add tests"
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3294562269)
Approach ACK 4fe3de2aa7fda58d5c56987aee13ae87191da1c6
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3294562269)
Approach ACK 4fe3de2aa7fda58d5c56987aee13ae87191da1c6
ðŽ vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398800190)
This assumes that all blocks at the same height have equal pskip height. This is true currently, but if changed, then this code will trigger the Assume in debug and will do null pointer dereference in release builds. Looking at `GetSkipHeight()`, it derives its result deterministically from `height` only (good).
Is it worth here, if `pa->pskip->nHeight != pb->pskip->nHeight`, then to fall back to one-by-one iterating?
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398800190)
This assumes that all blocks at the same height have equal pskip height. This is true currently, but if changed, then this code will trigger the Assume in debug and will do null pointer dereference in release builds. Looking at `GetSkipHeight()`, it derives its result deterministically from `height` only (good).
Is it worth here, if `pa->pskip->nHeight != pb->pskip->nHeight`, then to fall back to one-by-one iterating?
ðŽ vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398805880)
nit: I guess that if this check fails, then the test should be interrupted without performing further checks. If so, maybe use `BOOST_REQUIRE_EQUAL()`?
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398805880)
nit: I guess that if this check fails, then the test should be interrupted without performing further checks. If so, maybe use `BOOST_REQUIRE_EQUAL()`?
ðŽ vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398812630)
nit: similar as above: `BOOST_REQUIRE(a == b)` or `BOOST_REQUIRE_EQUAL(a, b)`
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398812630)
nit: similar as above: `BOOST_REQUIRE(a == b)` or `BOOST_REQUIRE_EQUAL(a, b)`
ðŽ vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398807972)
nit: similar as above: `BOOST_REQUIRE_EQUAL()`?
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398807972)
nit: similar as above: `BOOST_REQUIRE_EQUAL()`?
ðŽ ryanofsky commented on pull request "Update libmultiprocess subtree to support reduced logging":
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3361255971)
> I think the nix config is not using an instrumented libcpp, which could be a reason for the difference?
Yes, that was the reason! Enabling "-DLLVM_USE_SANITIZER=Thread" for libcxx in nix lets me reproduce this. Will make a PR updating the CI job and fixing the bug, once I figure out what the bug is
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3361255971)
> I think the nix config is not using an instrumented libcpp, which could be a reason for the difference?
Yes, that was the reason! Enabling "-DLLVM_USE_SANITIZER=Thread" for libcxx in nix lets me reproduce this. Will make a PR updating the CI job and fixing the bug, once I figure out what the bug is
ðŽ vasild commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-3361306154)
`a42449cb30...fc1ba52a4d`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-3361306154)
`a42449cb30...fc1ba52a4d`: rebase due to conflicts
ðŽ sipa commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398962673)
I don't think so - that eventuality (pskip depending on more than just height) is dealt with through the `Assume()`. I guess I can add a comment to clarify that.
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398962673)
I don't think so - that eventuality (pskip depending on more than just height) is dealt with through the `Assume()`. I guess I can add a comment to clarify that.
ðŽ Eunovo commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2398976897)
I also want to note that the generated blocks already contain `OP_RETURN` outputs in coinbase transactions, so we don't need to create a specific `OP_RETURN` output in the test.
This might also remove the need to have a second block at all, since there's no need to create a new tip for the `OP_RETURN` test
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2398976897)
I also want to note that the generated blocks already contain `OP_RETURN` outputs in coinbase transactions, so we don't need to create a specific `OP_RETURN` output in the test.
This might also remove the need to have a second block at all, since there's no need to create a new tip for the `OP_RETURN` test