💬 vasild commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1660036151)
_offtopic `[[nodiscard]]`_
There are 3 classes of functions wrt `[[nodiscard]]`:
1. where ignoring the result is harmful, it probably means a bug in the program, for example [write(2)](https://man7.org/linux/man-pages/man2/write.2.html)
2. where ignoring the result is ok and the code is equivalent without the call, aka it does not make sense to call the function, its call should be removed because it clutters the source code and makes the program slower, for example [strlen(3)](https://ma
...
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1660036151)
_offtopic `[[nodiscard]]`_
There are 3 classes of functions wrt `[[nodiscard]]`:
1. where ignoring the result is harmful, it probably means a bug in the program, for example [write(2)](https://man7.org/linux/man-pages/man2/write.2.html)
2. where ignoring the result is ok and the code is equivalent without the call, aka it does not make sense to call the function, its call should be removed because it clutters the source code and makes the program slower, for example [strlen(3)](https://ma
...
💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660036495)
> I'd be happy to work with a reopened #22242
I guess that pull is mostly orthogonal (shouldn't (silently) conflict with this one). Rebased and reopened in https://github.com/bitcoin/bitcoin/pull/28195
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660036495)
> I'd be happy to work with a reopened #22242
I guess that pull is mostly orthogonal (shouldn't (silently) conflict with this one). Rebased and reopened in https://github.com/bitcoin/bitcoin/pull/28195
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660128648)
@MarcoFalke, I am not sure I fully understand in [your comment](https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387). Can you elaborate on the CPFP example?
In general, wrt the privacy of the transaction originator, do you think that the "sensitive relay" mechanism from this PR could be worse than the current "send to all" from `master` in some cases? Or do you think that it is better than `master` but it can be done even better?
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660128648)
@MarcoFalke, I am not sure I fully understand in [your comment](https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387). Can you elaborate on the CPFP example?
In general, wrt the privacy of the transaction originator, do you think that the "sensitive relay" mechanism from this PR could be worse than the current "send to all" from `master` in some cases? Or do you think that it is better than `master` but it can be done even better?
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660192852)
> do you think that the "sensitive relay" mechanism from this PR could be worse
Yes, my understanding is that this pull strictly decreases privacy when the underlying wallet makes use of transactions chains (whether or not for intentional or accidental CPFP shouldn't matter), or transaction replacements, or mutually exclusive transactions ("conflicts", "double-spends" or insufficient rbf-fee), or if the transaction recipient is the attacker and can create transactions spending the "sensitive
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660192852)
> do you think that the "sensitive relay" mechanism from this PR could be worse
Yes, my understanding is that this pull strictly decreases privacy when the underlying wallet makes use of transactions chains (whether or not for intentional or accidental CPFP shouldn't matter), or transaction replacements, or mutually exclusive transactions ("conflicts", "double-spends" or insufficient rbf-fee), or if the transaction recipient is the attacker and can create transactions spending the "sensitive
...
👍 dergoegge approved a pull request: "refactor: Remove unused MessageStartChars parameters from BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/28191#pullrequestreview-1556884717)
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073
(https://github.com/bitcoin/bitcoin/pull/28191#pullrequestreview-1556884717)
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073
🤔 glozow reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1556437447)
Approach ACK, mempool sequence seems suitable for this.
I think we should add a functional test, perhaps [something like this](https://github.com/glozow/bitcoin/commit/258b1c8ced55b528d1afc585c23a3d98fa30394b)
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1556437447)
Approach ACK, mempool sequence seems suitable for this.
I think we should add a functional test, perhaps [something like this](https://github.com/glozow/bitcoin/commit/258b1c8ced55b528d1afc585c23a3d98fa30394b)
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280281473)
nit: this comment could be misleading after ffa081b007300f28fd9cffe6f4795c1125e2f373
```suggestion
const uint64_t entry_sequence; //!< Sequence number used to determine whether this transaction is too recent for relay.
```
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280281473)
nit: this comment could be misleading after ffa081b007300f28fd9cffe6f4795c1125e2f373
```suggestion
const uint64_t entry_sequence; //!< Sequence number used to determine whether this transaction is too recent for relay.
```
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280487599)
A comment might be helpful
```suggestion
/** Returns info for a transaction if its entry_sequence <= last_sequence. */
TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const;
```
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280487599)
A comment might be helpful
```suggestion
/** Returns info for a transaction if its entry_sequence <= last_sequence. */
TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const;
```
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280498101)
I think there may be an off-by-one here? The sequence number isn't incremented until `TransactionAddedToMempool()` fires later. Which means we could have:
t=0: mempool sequence is 1
t=1: tx A is submitted with `CTxMemPoolEntry` is constructed with `entry_sequence = m_pool.GetSequence() = 1`
t=2: main signals fires `TransactionAddedToMempool(A, m_pool.GetAndIncrementSequence())` with sequence 2
t=3: p2p announces A, `m_last_inv_sequence = m_mempool.GetSequence() = 2`
t=4: new tx B is submit
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280498101)
I think there may be an off-by-one here? The sequence number isn't incremented until `TransactionAddedToMempool()` fires later. Which means we could have:
t=0: mempool sequence is 1
t=1: tx A is submitted with `CTxMemPoolEntry` is constructed with `entry_sequence = m_pool.GetSequence() = 1`
t=2: main signals fires `TransactionAddedToMempool(A, m_pool.GetAndIncrementSequence())` with sequence 2
t=3: p2p announces A, `m_last_inv_sequence = m_mempool.GetSequence() = 2`
t=4: new tx B is submit
...
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280500977)
nit: the commit message for ffa081b007300f28fd9cffe6f4795c1125e2f373 is "validation: when adding txs due to a block reorg, allow immediate relay" but the immediate relay behavior isn't until the subsequent commit when the bloom filter is removed
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280500977)
nit: the commit message for ffa081b007300f28fd9cffe6f4795c1125e2f373 is "validation: when adding txs due to a block reorg, allow immediate relay" but the immediate relay behavior isn't until the subsequent commit when the bloom filter is removed
💬 fanquake commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1280574653)
I think we should avoid adding comments like this, because they don't really provide any value to the developer. If anything, a comment explaining what the "not normal" case is, and/or why that's particularly relevant, would be more useful, because that's the first question I'd have after reading this comment. Also unsure why this code has been re-ordered.
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1280574653)
I think we should avoid adding comments like this, because they don't really provide any value to the developer. If anything, a comment explaining what the "not normal" case is, and/or why that's particularly relevant, would be more useful, because that's the first question I'd have after reading this comment. Also unsure why this code has been re-ordered.
🤔 dergoegge reviewed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1554560960)
I think this approach is better suited for a separate tool. Otherwise fingerprinting becomes a direct vector for tx origin analysis (see my inline comments), which seems like a regression from what we have now.
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1554560960)
I think this approach is better suited for a separate tool. Otherwise fingerprinting becomes a direct vector for tx origin analysis (see my inline comments), which seems like a regression from what we have now.
💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279166534)
this should go into `net_processing.h` where it is used
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279166534)
this should go into `net_processing.h` where it is used
💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279194945)
Using the txid here can leak the tx origin.
e.g. an attacker changes the witness of your tx (such that the tx is invalid) and sends it to you, if you then announce the original tx to your peers it is obvious that the tx originated from you.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279194945)
Using the txid here can leak the tx origin.
e.g. an attacker changes the witness of your tx (such that the tx is invalid) and sends it to you, if you then announce the original tx to your peers it is obvious that the tx originated from you.
💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279343481)
What about children of local txs? They currently leak tx origin as well.
They should be treated as orphans until the parent tx is scheduled for relay to all.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279343481)
What about children of local txs? They currently leak tx origin as well.
They should be treated as orphans until the parent tx is scheduled for relay to all.
💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279206967)
Unless I'm missing something, the pings on sensitive connections never timeout. So if the peer doesn't respond to the ping the connection only closes after 20min (`TIMEOUT_INTERVAL`).
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279206967)
Unless I'm missing something, the pings on sensitive connections never timeout. So if the peer doesn't respond to the ping the connection only closes after 20min (`TIMEOUT_INTERVAL`).
💬 dergoegge commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279227800)
Not processing messages that are irrelevant to this type of tx relay is the right move but it still leaves room for tx origin leakage through fingerprinting in the version handshake.
In other words, if it is possible to fingerprint a node solely based on version handshake messages then it is possible for the receiver of a "sensitive-relay" tx to figure out which public addresses belong to the node that broadcast the tx.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279227800)
Not processing messages that are irrelevant to this type of tx relay is the right move but it still leaves room for tx origin leakage through fingerprinting in the version handshake.
In other words, if it is possible to fingerprint a node solely based on version handshake messages then it is possible for the receiver of a "sensitive-relay" tx to figure out which public addresses belong to the node that broadcast the tx.
💬 fanquake commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1660241633)
@MarcoFalke does this fix #28057 in it's current state?
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1660241633)
@MarcoFalke does this fix #28057 in it's current state?
💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280577467)
In 87abceea:
Maybe better `WALLET_FLAG_GLOBAL_HD_KEY`?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280577467)
In 87abceea:
Maybe better `WALLET_FLAG_GLOBAL_HD_KEY`?
💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280568616)
Or.. another possible patch to not mix the keys reading code with the upgrade code could be to read the xpub root key alone at the end of the spkm loading process:
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
--- a/src/wallet/walletdb.cpp (revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
+++ b/src/wallet/walletdb.cpp (date 1690893250659)
@@ -966,6 +966,31 @@
result = std::max(result, ckey_res.m_result);
num_ckeys = ckey_res.m_records;
+
...
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1280568616)
Or.. another possible patch to not mix the keys reading code with the upgrade code could be to read the xpub root key alone at the end of the spkm loading process:
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
--- a/src/wallet/walletdb.cpp (revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
+++ b/src/wallet/walletdb.cpp (date 1690893250659)
@@ -966,6 +966,31 @@
result = std::max(result, ckey_res.m_result);
num_ckeys = ckey_res.m_records;
+
...
💬 sipa commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660269660)
I think the only correct approach to implement something like this, is to keep the unbroadcast transaction within the wallet (in a special unbroadcast state until it's echoed back). In this unbroadcast state, rebroadcasts can happen (directly to sensitive relay peers only, bypassing the mempool), but outputs are not available for creating descendant transactions.
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660269660)
I think the only correct approach to implement something like this, is to keep the unbroadcast transaction within the wallet (in a special unbroadcast state until it's echoed back). In this unbroadcast state, rebroadcasts can happen (directly to sensitive relay peers only, bypassing the mempool), but outputs are not available for creating descendant transactions.