💬 stickies-v commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392038207)
The commit from which this is backported just added an extra zero:
https://github.com/bitcoin/bitcoin/commit/6da5de58cabc4133c379baa50845e30e5bc6b3e4#diff-63dc84ee23b871d1f4931c4f261b3c6b815bd8d5a65098a61bce8ea9b6b81965
What's the reason for changing that to dividing by 2 here? It also doesn't seem necessary for the test to complete.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392038207)
The commit from which this is backported just added an extra zero:
https://github.com/bitcoin/bitcoin/commit/6da5de58cabc4133c379baa50845e30e5bc6b3e4#diff-63dc84ee23b871d1f4931c4f261b3c6b815bd8d5a65098a61bce8ea9b6b81965
What's the reason for changing that to dividing by 2 here? It also doesn't seem necessary for the test to complete.
🤔 maflcko reviewed a pull request: "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3285972950)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3285972950)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392299387)
the file can be deleted now?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392299387)
the file can be deleted now?
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301282)
method can be deleted now?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301282)
method can be deleted now?
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301711)
what is this for?
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392301711)
what is this for?
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392309486)
why make this uppercase? could just leave as-is to make the diff smaller
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392309486)
why make this uppercase? could just leave as-is to make the diff smaller
💬 glozow commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392363907)
I think you are right that changing the `Decimal` would have worked - apologies for the confusing change in approach.
This test doesn't exist on master, so it isn't part of the backport. It's a conflict resolution - the test was adapted to make it pass on that commit. I think you're referring to a different line lower down (L315) where it's backported and adds a 0.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2392363907)
I think you are right that changing the `Decimal` would have worked - apologies for the confusing change in approach.
This test doesn't exist on master, so it isn't part of the backport. It's a conflict resolution - the test was adapted to make it pass on that commit. I think you're referring to a different line lower down (L315) where it's backported and adds a 0.
💬 glozow commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3353195323)
> nit: https://github.com/bitcoin/bitcoin/commit/e3273e03b1aea0c3ee3aeca802c984ab007f91ed and https://github.com/bitcoin/bitcoin/commit/08eeb0d3423cdfb6110794dd2bfdd5571459f751 have two spaces after Rebased-From: instead of the usual one, I'm not sure if that'd break any tooling (it did for me, but was an easy fix)
Sorry about that! I hadn't really thought about the number of spaces, but will keep it in mind in the future.
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3353195323)
> nit: https://github.com/bitcoin/bitcoin/commit/e3273e03b1aea0c3ee3aeca802c984ab007f91ed and https://github.com/bitcoin/bitcoin/commit/08eeb0d3423cdfb6110794dd2bfdd5571459f751 have two spaces after Rebased-From: instead of the usual one, I'm not sure if that'd break any tooling (it did for me, but was an easy fix)
Sorry about that! I hadn't really thought about the number of spaces, but will keep it in mind in the future.
💬 achow101 commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3353204914)
ACK fc861332b351c9390400054ff74193ce26eb0713
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3353204914)
ACK fc861332b351c9390400054ff74193ce26eb0713
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392383472)
I don't think it is necessary to check this, we can assume that libsecp is working correctly.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392383472)
I don't think it is necessary to check this, we can assume that libsecp is working correctly.
🤔 mzumsande reviewed a pull request: "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value"
(https://github.com/bitcoin/bitcoin/pull/33498#pullrequestreview-3286046607)
Concept ACK
The suggested solution reduces the precision of timestamps for the receiving party, but makes fingerprinting harder - seems like an acceptable trade-off to me.
If there is an active node behind the address it won't be `Terrible` for another ~20 days - until that happens, we could update it with a more accurate timestamp when we connect to it or receive the addr via gossip relay.
(https://github.com/bitcoin/bitcoin/pull/33498#pullrequestreview-3286046607)
Concept ACK
The suggested solution reduces the precision of timestamps for the receiving party, but makes fingerprinting harder - seems like an acceptable trade-off to me.
If there is an active node behind the address it won't be `Terrible` for another ~20 days - until that happens, we could update it with a more accurate timestamp when we connect to it or receive the addr via gossip relay.
💬 mzumsande commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2392350010)
Since this is calculated on a level of minutes, it looks like it would give a range 10.5±2.5 days, not 10±2 days.
(https://github.com/bitcoin/bitcoin/pull/33498#discussion_r2392350010)
Since this is calculated on a level of minutes, it looks like it would give a range 10.5±2.5 days, not 10±2 days.
💬 davidgumberg commented on pull request "depends: static libxcb-cursor":
(https://github.com/bitcoin/bitcoin/pull/33434#issuecomment-3353249464)
@hebasto The machine that gets this error is an x86_64 running Fedora 42, the issue recurs on this machine, but I am in the process of setting up Guix on some other devices to test.
(https://github.com/bitcoin/bitcoin/pull/33434#issuecomment-3353249464)
@hebasto The machine that gets this error is an x86_64 running Fedora 42, the issue recurs on this machine, but I am in the process of setting up Guix on some other devices to test.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392417001)
> A more practical scenario would be where the `MuSig2` unspents are combined with non-MuSig2/Taproot unspents
I don't think multisig inputs tend to be mixed with non-multisig inputs.
> A secondary reason is to avoid seeing `dec_psbt["inputs"][0]` in several places.
I prefer to have these tests check the input that we know is musig rather than searching for it dynamically.
> Iterates all the test cases over `ALL|ANYONECANPAY` sighash type.
There's nothing special about `ALL|ANYON
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392417001)
> A more practical scenario would be where the `MuSig2` unspents are combined with non-MuSig2/Taproot unspents
I don't think multisig inputs tend to be mixed with non-multisig inputs.
> A secondary reason is to avoid seeing `dec_psbt["inputs"][0]` in several places.
I prefer to have these tests check the input that we know is musig rather than searching for it dynamically.
> Iterates all the test cases over `ALL|ANYONECANPAY` sighash type.
There's nothing special about `ALL|ANYON
...
🚀 achow101 merged a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299)
(https://github.com/bitcoin/bitcoin/pull/33299)
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392420315)
Yes, i believe so, since the json data is no longer being cached, this becomes stale
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392420315)
Yes, i believe so, since the json data is no longer being cached, this becomes stale
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392424606)
Yes, this is no longer needed and can be deleted
Done
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392424606)
Yes, this is no longer needed and can be deleted
Done
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392425527)
Reverted to make diff smaller
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392425527)
Reverted to make diff smaller
💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3353283982)
> Have you seen #29415? Is there a reason you would want this if we had private broadcast?
Yes! I saw it while reviewing the issues; it is a really nice privacy solution, but still I think this can have some use-cases.
Apart from testing/simulating network stuff, it can be used in an environment where your tx may be dropped/filtered by some of your peers. In that case, the ability to manually select one peer that you know will accept it and route it can be useful while preserving some priv
...
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3353283982)
> Have you seen #29415? Is there a reason you would want this if we had private broadcast?
Yes! I saw it while reviewing the issues; it is a really nice privacy solution, but still I think this can have some use-cases.
Apart from testing/simulating network stuff, it can be used in an environment where your tx may be dropped/filtered by some of your peers. In that case, the ability to manually select one peer that you know will accept it and route it can be useful while preserving some priv
...
💬 maflcko commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3353310087)
concept ack. This should help fix https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3011875412, via https://codeberg.org/guix/guix/commit/a9c33e9f688fce88aed610ab04c650efb71b4ce6, which is included in https://codeberg.org/guix/guix/commit/5cb84f2013c5b1e48a7d0e617032266f1e6059e2
I haven't looked at the underlying dependency graph changes, but this seems reasonable.
```
# uname --machine && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C s
...
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3353310087)
concept ack. This should help fix https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-3011875412, via https://codeberg.org/guix/guix/commit/a9c33e9f688fce88aed610ab04c650efb71b4ce6, which is included in https://codeberg.org/guix/guix/commit/5cb84f2013c5b1e48a7d0e617032266f1e6059e2
I haven't looked at the underlying dependency graph changes, but this seems reasonable.
```
# uname --machine && find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C s
...