💬 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
...
💬 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_r2392462021)
Done
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392462021)
Done
💬 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_r2392462361)
It's no longer relevant, removed.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392462361)
It's no longer relevant, removed.
💬 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_r2392462540)
Done
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392462540)
Done
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392492431)
Was supposed to be for the test to work in non wallet environments, but since the test now always uses MiniWallet internally by default, this is no longer needed
Removed the line of code
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392492431)
Was supposed to be for the test to work in non wallet environments, but since the test now always uses MiniWallet internally by default, this is no longer needed
Removed the line of code
📝 ryanofsky opened a pull request: "init: Signal m_tip_block_cv on Ctrl-C"
(https://github.com/bitcoin/bitcoin/pull/33511)
Signal `m_tip_block_cv` when Ctrl-C is pressed or `SIGTERM` is received, the same way it is currently signaled when the `stop` RPC is called. This lets RPC calls like `waitforblockheight` and IPC calls like `waitTipChanged` be interrupted, instead of waiting for their original timeouts and delaying shutdown.
This issue was reported by plebhash in #33463
(https://github.com/bitcoin/bitcoin/pull/33511)
Signal `m_tip_block_cv` when Ctrl-C is pressed or `SIGTERM` is received, the same way it is currently signaled when the `stop` RPC is called. This lets RPC calls like `waitforblockheight` and IPC calls like `waitTipChanged` be interrupted, instead of waiting for their original timeouts and delaying shutdown.
This issue was reported by plebhash in #33463
🤔 l0rinc reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3282387933)
I'm not immersed in this to give a meaningful ack yet, but please see a few of my comments.
My main observation is that we're introducing tests and new code at the same time and I find it hard to follow what the relation between the two is - how do I tell if they're doing what they're supposed to.
Given that this is meant to replace libevent, I wonder if we could add either a wrapper to libevent and cover it with tests as a first step (could be done in a separate standalone PR as well) and slow
...
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3282387933)
I'm not immersed in this to give a meaningful ack yet, but please see a few of my comments.
My main observation is that we're introducing tests and new code at the same time and I find it hard to follow what the relation between the two is - how do I tell if they're doing what they're supposed to.
Given that this is meant to replace libevent, I wonder if we could add either a wrapper to libevent and cover it with tests as a first step (could be done in a separate standalone PR as well) and slow
...
💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389700626)
nit: I know this was the original name as well, but variables (by their name) should reflect the purpose of their usage, not their value. `ipv6_only{1}` or `tcp_nodelay{1}` might make the context more obvious (if that's indeed what they're goal is.
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389700626)
nit: I know this was the original name as well, but variables (by their name) should reflect the purpose of their usage, not their value. `ipv6_only{1}` or `tcp_nodelay{1}` might make the context more obvious (if that's indeed what they're goal is.