✅ hebasto closed a pull request: "refactor, wallet: Avoid variable shadowing"
(https://github.com/bitcoin/bitcoin/pull/27087)
(https://github.com/bitcoin/bitcoin/pull/27087)
💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1104560393)
Thanks, updated.
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1104560393)
Thanks, updated.
💬 sipa commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428054418)
> I agree with @sipa, with a stronger emphasis that I would probably NACK this change because the cost of this fix is too high, and the privacy gain is too low.
Yeah, Approach NACK. I may be convinced that doing *something* to avoid mempool fingerprinting through GETBLOCKTXN is worth it, but if we want that, there are better ways than this.
> Note that it's very easy for an adversary to change that by simply broadcasting simultaneous double-spends with the same fee. Indeed, n-way double sp
...
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428054418)
> I agree with @sipa, with a stronger emphasis that I would probably NACK this change because the cost of this fix is too high, and the privacy gain is too low.
Yeah, Approach NACK. I may be convinced that doing *something* to avoid mempool fingerprinting through GETBLOCKTXN is worth it, but if we want that, there are better ways than this.
> Note that it's very easy for an adversary to change that by simply broadcasting simultaneous double-spends with the same fee. Indeed, n-way double sp
...
💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104563193)
Why only for bit-wise operations? It's just a standard data type that represents an unsigned 64-bit integer.
Didn't have any specific reason, just that is more concise and specific. Could change it to a bare unsigned int too.
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104563193)
Why only for bit-wise operations? It's just a standard data type that represents an unsigned 64-bit integer.
Didn't have any specific reason, just that is more concise and specific. Could change it to a bare unsigned int too.
💬 MarcoFalke commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428059849)
> If we don't add wallet transactions to the mempool but still relay them
Yeah, I didn't mention this, but obviously we wouldn't relay them with the mempool. Doing a one-shot (tor-only) outbound connection to fan-out the tx (one-hop dandelion) without adding it to the mempool shouldn't leave a fingerprint, other than the one left by the tor-only connection, no?
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428059849)
> If we don't add wallet transactions to the mempool but still relay them
Yeah, I didn't mention this, but obviously we wouldn't relay them with the mempool. Doing a one-shot (tor-only) outbound connection to fan-out the tx (one-hop dandelion) without adding it to the mempool shouldn't leave a fingerprint, other than the one left by the tor-only connection, no?
💬 sipa commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428064176)
@MarcoFalke Oh, fair enough, that's a good idea (though it'd probably still need a fallback to normal relay after some delay if we don't observe the transaction being rumoured back to us). I also think it's orthogonal to the idea here, because even absent "first mile" wallet broadcast leakage, we still want the P2P network to obscure transaction relay beyond that.
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428064176)
@MarcoFalke Oh, fair enough, that's a good idea (though it'd probably still need a fallback to normal relay after some delay if we don't observe the transaction being rumoured back to us). I also think it's orthogonal to the idea here, because even absent "first mile" wallet broadcast leakage, we still want the P2P network to obscure transaction relay beyond that.
💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104574509)
pushed
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104574509)
pushed
💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104574758)
pushed
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104574758)
pushed
💬 furszy commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104575040)
pushed
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1104575040)
pushed
💬 stickies-v commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104579180)
I did some digging, and I think (would be nice to get more confirmation on this) we can scrap the `net_specific == false` version entirely, we don't seem to need it. `EnsureDataDir` is called 3 times: 2 times right before reading config files - but it looks like we don't actually need to ensure a datadir there. If datadir doesn't exist, by definition no config files can be read (and that's handled gracefully). The only time we need it is where `net_specific==true`.
Intuitively, this makes sen
...
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104579180)
I did some digging, and I think (would be nice to get more confirmation on this) we can scrap the `net_specific == false` version entirely, we don't seem to need it. `EnsureDataDir` is called 3 times: 2 times right before reading config files - but it looks like we don't actually need to ensure a datadir there. If datadir doesn't exist, by definition no config files can be read (and that's handled gracefully). The only time we need it is where `net_specific==true`.
Intuitively, this makes sen
...
💬 fanquake commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1428074269)
Does/can this also close #16220?
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1428074269)
Does/can this also close #16220?
💬 MarcoFalke commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428093567)
> we still want the P2P network to obscure transaction relay beyond that
I wonder if that is worth it. Given this issue here (and past ones), it just seems hard to think about and any guarantees are at best brittle in an evolving P2P network. So, long term, assuming the private "first mile" privacy-preserving fan out stuff is available, users and wallets caring about it will probably use that. Attempts to optimize the normal relay to be equally privacy-preserving will always have a taste of a
...
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428093567)
> we still want the P2P network to obscure transaction relay beyond that
I wonder if that is worth it. Given this issue here (and past ones), it just seems hard to think about and any guarantees are at best brittle in an evolving P2P network. So, long term, assuming the private "first mile" privacy-preserving fan out stuff is available, users and wallets caring about it will probably use that. Attempts to optimize the normal relay to be equally privacy-preserving will always have a taste of a
...
💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1428104313)
> Does/can this also close #16220?
I will check this out, specifically w.r.t this comment which I had not been considering:
> It would be, however the reason that wallets/ is not created when the .bitcoin directory already exists is backwards compatibility. If there is an existing datadir but no wallets directory it's assumed to be an old data directory, and creating wallets would effectively hide the existing wallets in the top level dir, possibly leading to people to suspect loss of fund
...
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1428104313)
> Does/can this also close #16220?
I will check this out, specifically w.r.t this comment which I had not been considering:
> It would be, however the reason that wallets/ is not created when the .bitcoin directory already exists is backwards compatibility. If there is an existing datadir but no wallets directory it's assumed to be an old data directory, and creating wallets would effectively hide the existing wallets in the top level dir, possibly leading to people to suspect loss of fund
...
💬 jonatack commented on pull request "Minor edits - punctuation, spelling to make the contributing instructions more readable for all.":
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1103668241)
This change is incorrect.
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1103668241)
This change is incorrect.
💬 jonatack commented on pull request "Minor edits - punctuation, spelling to make the contributing instructions more readable for all.":
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1103668412)
Incorrect change.
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1103668412)
Incorrect change.
💬 jonatack commented on pull request "Minor edits - punctuation, spelling to make the contributing instructions more readable for all.":
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1103668222)
This incorrectly changes the intended meaning.
(https://github.com/bitcoin/bitcoin/pull/27082#discussion_r1103668222)
This incorrectly changes the intended meaning.
💬 sipa commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428116350)
We can't rely on Tor for all wallet privacy, especially given that it's a centralized service that might just fail completely one day (and before that, it's hard to bound how much sufficiently powerful attackers can learn from traffic analysis in Tor).
Privacy on a public network is always multi-faceted, and it's fair we can't make sure many guarantees. But on the other hand, we go through pretty substantial efforts to hide *lots* of things on a best-effort basis, especially involving transac
...
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1428116350)
We can't rely on Tor for all wallet privacy, especially given that it's a centralized service that might just fail completely one day (and before that, it's hard to bound how much sufficiently powerful attackers can learn from traffic analysis in Tor).
Privacy on a public network is always multi-faceted, and it's fair we can't make sure many guarantees. But on the other hand, we go through pretty substantial efforts to hide *lots* of things on a best-effort basis, especially involving transac
...
💬 hebasto commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1104616746)
> Conditional calls to AC_DEFINE (such as those setting it to a constant `1`, or not setting a value at all) need `defined()`/`#ifdef` AIUI
Agree with @luke-jr.
In this particular case, it is conditional: https://github.com/bitcoin/bitcoin/blob/8126551d54ffd290ed5767248be4b3d19243787b/configure.ac#L1707-L1710
therefore, the current code is fine.
(https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1104616746)
> Conditional calls to AC_DEFINE (such as those setting it to a constant `1`, or not setting a value at all) need `defined()`/`#ifdef` AIUI
Agree with @luke-jr.
In this particular case, it is conditional: https://github.com/bitcoin/bitcoin/blob/8126551d54ffd290ed5767248be4b3d19243787b/configure.ac#L1707-L1710
therefore, the current code is fine.
💬 jonatack commented on pull request "cli: include local ("unroutable") peers in -netinfo table":
(https://github.com/bitcoin/bitcoin/pull/26584#issuecomment-1428125914)
This is not a risky change and seems fine to merge.
(https://github.com/bitcoin/bitcoin/pull/26584#issuecomment-1428125914)
This is not a risky change and seems fine to merge.
💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104628124)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104282294
> Allowing for a `void` failure type seems to make this incompatible to be switched out to `std::expected` https://en.cppreference.com/w/cpp/utility/expected ?
>
> With multiple warning and error messages this may already be incompatible, though?
Yes, if you are thinking that `util::Result` could wrap `std::expected` that would be probably be awkward and not worth it.
But I don't think there is a conflict becaus
...
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104628124)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104282294
> Allowing for a `void` failure type seems to make this incompatible to be switched out to `std::expected` https://en.cppreference.com/w/cpp/utility/expected ?
>
> With multiple warning and error messages this may already be incompatible, though?
Yes, if you are thinking that `util::Result` could wrap `std::expected` that would be probably be awkward and not worth it.
But I don't think there is a conflict becaus
...
💬 brunoerg commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#discussion_r1104630727)
Since you're replacing `base64` lib to use `secrets`, perhaps would be good to do the same in `rpcauth-test`?
(https://github.com/bitcoin/bitcoin/pull/27081#discussion_r1104630727)
Since you're replacing `base64` lib to use `secrets`, perhaps would be good to do the same in `rpcauth-test`?