🤔 pinheadmz reviewed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1458209334)
concept ACK
Reviewed all code - really great job with this! Very cool idea. I will continue to track this PR and look forward to testing it out more and trying to break the functional tests ;-)
I have a few questions below about implementation
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1458209334)
concept ACK
Reviewed all code - really great job with this! Very cool idea. I will continue to track this PR and look forward to testing it out more and trying to break the functional tests ;-)
I have a few questions below about implementation
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214690639)
Just so I understand: this change is required because with the PR we wouldn't send our own TX in response to a `MEMPOOL` message, so you quickly spin up a dummy peer to send the TX to us first. Then we can assert that it was relayed to `fitler_peer` ?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214690639)
Just so I understand: this change is required because with the PR we wouldn't send our own TX in response to a `MEMPOOL` message, so you quickly spin up a dummy peer to send the TX to us first. Then we can assert that it was relayed to `fitler_peer` ?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214675802)
Why is this extra ping needed for sensitive connections ?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214675802)
Why is this extra ping needed for sensitive connections ?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214698071)
Super cool idea! Is there common chance for failure here though? This relies on a chain of peers from the sensitive relay node back to us that haven't seen the tx yet.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214698071)
Super cool idea! Is there common chance for failure here though? This relies on a chain of peers from the sensitive relay node back to us that haven't seen the tx yet.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214671917)
Why use a funny value here instead of our actual subver? I'm guessing it's for extra anonymity? Could there be any downsides though, like nodes that refuse connections to certain agents?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214671917)
Why use a funny value here instead of our actual subver? I'm guessing it's for extra anonymity? Could there be any downsides though, like nodes that refuse connections to certain agents?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214678210)
Why disconnect the sensitive connection on PONG? Could that ever happen before we send our tx messages out?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214678210)
Why disconnect the sensitive connection on PONG? Could that ever happen before we send our tx messages out?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214696791)
nit: don't we usually do this kind of logic in one line?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214696791)
nit: don't we usually do this kind of logic in one line?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214684616)
I was just about to ask! Wouldn't this only hurt users who don't have sensitive relay available? Maybe `MEMPOOL` requests don't matter so much but wouldn't omitting from `GETDATA` prevent our own TX from being broadcast at all?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214684616)
I was just about to ask! Wouldn't this only hurt users who don't have sensitive relay available? Maybe `MEMPOOL` requests don't matter so much but wouldn't omitting from `GETDATA` prevent our own TX from being broadcast at all?
💬 kevkevinpal commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574189200)
>
ya that could work and we wouldn't need to add a whole new rpc. That would require us to make the fee_delta param optional and if priotirisetransaction(<txid>, 0) we can check if no fee_delta and the param = 0 we set the map delta to 0
not sure if its forbidden to modify that dummy param not exactly sure it is a dummy param or its history
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1574189200)
>
ya that could work and we wouldn't need to add a whole new rpc. That would require us to make the fee_delta param optional and if priotirisetransaction(<txid>, 0) we can check if no fee_delta and the param = 0 we set the map delta to 0
not sure if its forbidden to modify that dummy param not exactly sure it is a dummy param or its history
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214696267)
I think this introduces an interdependency between the number of outgoing transaction bytes we can receive from a peer and the maximum flow of LN commitment transaction+CPFP in weight a direct transaction-relay peer can announce to us and therefore we can validate. An `option_anchors_zero_fee_htlc_tx` LN commitment transaction weight is 900 WU with no pending HTLCs. As the orphan memory usage is implemented, it’s unclear if there is any processing guarantees for a peer announcing transaction to
...
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214696267)
I think this introduces an interdependency between the number of outgoing transaction bytes we can receive from a peer and the maximum flow of LN commitment transaction+CPFP in weight a direct transaction-relay peer can announce to us and therefore we can validate. An `option_anchors_zero_fee_htlc_tx` LN commitment transaction weight is 900 WU with no pending HTLCs. As the orphan memory usage is implemented, it’s unclear if there is any processing guarantees for a peer announcing transaction to
...
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214720397)
If I understand correctly the rate-limiting accounting introduced here, whatever the number of orphan parent missing, there will be an in-flight orphan parent requests issued (i.e if we’re missing 10 parents, we’re going to issue 10 in-flight orphan parent requests).
I think this should be documented as multi-party applications and contracting protocols transactions issuers might have to assemble their packages in function.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214720397)
If I understand correctly the rate-limiting accounting introduced here, whatever the number of orphan parent missing, there will be an in-flight orphan parent requests issued (i.e if we’re missing 10 parents, we’re going to issue 10 in-flight orphan parent requests).
I think this should be documented as multi-party applications and contracting protocols transactions issuers might have to assemble their packages in function.
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214712117)
There is a argument that can be made if memory and throughputs limits (e.g `MAX_PEER_ANNOUNCEMENTS`) are applied in function of the type of transactions received (e.g v3 transactions) and that can be used to nudge multi-party and contracting protocols transactions broadcasters to use fee-bumping efficient mechanism such as one CPFP covering multiple parents due to the package limits. This would be a discount incentive by analogy with SegWit spending discounted by consensus validation rules.
O
...
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214712117)
There is a argument that can be made if memory and throughputs limits (e.g `MAX_PEER_ANNOUNCEMENTS`) are applied in function of the type of transactions received (e.g v3 transactions) and that can be used to nudge multi-party and contracting protocols transactions broadcasters to use fee-bumping efficient mechanism such as one CPFP covering multiple parents due to the package limits. This would be a discount incentive by analogy with SegWit spending discounted by consensus validation rules.
O
...
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214728153)
This is unclear what level of enforcement is granted to a “final” decision, if the transactions are added to any transaction-relay download filters such as `m_recent_rejects` or `m_recently_announced_invs`. Otherwise, in case of blocks re-orgs dropping out the first package component and a second package component still stuck in filters, this can be source of package propagation failure.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214728153)
This is unclear what level of enforcement is granted to a “final” decision, if the transactions are added to any transaction-relay download filters such as `m_recent_rejects` or `m_recently_announced_invs`. Otherwise, in case of blocks re-orgs dropping out the first package component and a second package component still stuck in filters, this can be source of package propagation failure.
💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214732393)
I think there can be a source of concerns if all the announced orphans are second components from a single common parent. All our peers would announce those second-stage components, only one of them will succeed in the mempool validation and other will be refused as conflicts, I think.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214732393)
I think there can be a source of concerns if all the announced orphans are second components from a single common parent. All our peers would announce those second-stage components, only one of them will succeed in the mempool validation and other will be refused as conflicts, I think.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214738570)
(I just realized I answered my own question from https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214675802)
But still, don't we also send automatic PINGs ? Could that result in premature disconnection?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214738570)
(I just realized I answered my own question from https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214675802)
But still, don't we also send automatic PINGs ? Could that result in premature disconnection?
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214751567)
Orphan usage would effect the CPFP transactions, IIUC, not the commitment transactions. The CPFP is deemed high enough, sent to the peer, the peer holds the child tx in orphan set and requests the full package, which isn't size-bound(beyond current mempool limits).
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1214751567)
Orphan usage would effect the CPFP transactions, IIUC, not the commitment transactions. The CPFP is deemed high enough, sent to the peer, the peer holds the child tx in orphan set and requests the full package, which isn't size-bound(beyond current mempool limits).
💬 brunoerg commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1214786122)
nit:
```suggestion
if blockmintxfee_btc_kvb > 0: # can't go below zero-fee
```
I think `>` fits better with the comment "can't go below zero-fee" then `!=`
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1214786122)
nit:
```suggestion
if blockmintxfee_btc_kvb > 0: # can't go below zero-fee
```
I think `>` fits better with the comment "can't go below zero-fee" then `!=`
👍 brunoerg approved a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1458443810)
crACK 989fa37670cbb386873a765799fd6db8ea6986f7
Nice test coverage!
Just a thought that came to my mind while reviewing it:
- Seems like these test coverage uses just 1 node, if we'd stop node1 right before line 81, it wouldn't make the test to fail. So, it seems that `-minrelaytxfee=0` is just to make `send_self_transfer` - which calls `sendrawtransaction` - not fail (even if we might not have any connection).
(https://github.com/bitcoin/bitcoin/pull/27620#pullrequestreview-1458443810)
crACK 989fa37670cbb386873a765799fd6db8ea6986f7
Nice test coverage!
Just a thought that came to my mind while reviewing it:
- Seems like these test coverage uses just 1 node, if we'd stop node1 right before line 81, it wouldn't make the test to fail. So, it seems that `-minrelaytxfee=0` is just to make `send_self_transfer` - which calls `sendrawtransaction` - not fail (even if we might not have any connection).
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1574302283)
Correct me please if I'm missing anything. Considering:
```
- Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.
- After we receive the full transaction as a TX message, in reply to our GETDATA request, only then consider the transaction has propagated through the network and remove it from the unbroadcast list.
```
and
```
We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not t
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1574302283)
Correct me please if I'm missing anything. Considering:
```
- Once we get an INV from somebody, request the transaction with GETDATA, as if we didn't have it before.
- After we receive the full transaction as a TX message, in reply to our GETDATA request, only then consider the transaction has propagated through the network and remove it from the unbroadcast list.
```
and
```
We could wait for more than one INV, from peers that we have selected (outbound) and that are specifically not t
...
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214831451)
It seems for extra anonymity, but I have a similar doubt, couldn't I make easier for someone to censor us?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214831451)
It seems for extra anonymity, but I have a similar doubt, couldn't I make easier for someone to censor us?