search

lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client
MIT License
907 stars 182 forks source link

Neutrino seems to be expecting a MsgReject when broadcasting a tx, but btcd does not reply to the INV #206

Closed cjdelisle closed 3 years ago

cjdelisle commented 4 years ago

In order to decide when to stop rebroadcasting, Broadcaster is expecting to see a MsgReject. But I see that broadcaster calls cfg.Broadcast which ends in ChainService.sendTransaction() and this is sending an INV message. On the btcd side, INV messages which are already known in any way are quietly ignored, as you can see in SyncManager.handleInvMessage() so unless I am mistaken, this idea of rebroadcasting until someone rejects as duplicate is not just non-ideal, it actually doesn't work in the normal case (unless bitcoind works differently, in which a case could be made that this is btcd's bug).

Roasbeef commented 4 years ago

this idea of rebroadcasting until someone rejects as duplicate is not just non-ideal, it actually doesn't work in the normal case (unless bitcoind works differently

Correct we added that just as a heuristic to know when to stop early . In reality, one should only stop rebroadcasting once the transaction has been confirmed in the chain.

cjdelisle commented 4 years ago

Am I correct that the there is no code-path to tell Neutrino to stop broadcasting, except the one triggered by getting back a 'conflict' rejection ? What I'm seeing is that broadcasts pile up forever, but I'm not sure if it is something I might have broken by my changes to the codebase.

wpaulino commented 4 years ago

Correct. The broadcaster is stateless, on restart wallet transactions get queued. Ideally, we wait for the transaction to be included in a block to stop broadcasting, but that has yet to be done.

halseth commented 4 years ago

Also must keep in mind that newer bitcoind versions will never send reject messages.

Roasbeef commented 3 years ago

In reality, one should only stop rebroadcasting once the transaction has been confirmed in the chain.

Seems like we only need to implement this in order to close this gap?

yaslama commented 3 years ago

In reality, one should only stop rebroadcasting once the transaction has been confirmed in the chain.

Seems like we only need to implement this in order to close this gap?

@Roasbeef Do you plan to implement that in the near future? We also see that broadcasts pile up forever when using bitcoind.

Roasbeef commented 3 years ago

@yaslama feel free to pick it up.

kingonly commented 3 years ago

@Roasbeef this isn't an easy bug to pick up... This should take a higher priority, shouldn't it? Seems like a serious bug and perhaps the title doesn't reflect its severity. Correct me if I'm wrong, but with the latest bitcoind, anyone using Neutrino to broadcast a tx, keeps broadcasting it forever, even if it's already confirmed.

wpaulino commented 3 years ago

I took a quick look at how this could be implemented and it could definitely be non-trivial based on the approach taken. To make sure we detect transactions confirmed while offline, we'd have to keep track of a height hint for each transaction, as similarly done by the ChainNotifier in lnd. For the sake of improving neutrino as a standalone library (without the use of lnd), I think it would be best for it to follow that approach, either by integrating the ChainNotifier or replicating its behavior.

Given the severity of this issue affecting all neutrino lnd users, we could also go with a more trivial approach. All transactions broadcast by lnd go through its wallet. Since the wallet can already detect when a transaction confirms, we can provide a hook to cancel the transaction broadcast when running with a neutrino backend. The downside to this approach is that it wouldn't address the issue when neutrino is being used without lnd.

Thoughts?

Roasbeef commented 3 years ago

Are all the rebroadcast attempts y'all re seeing transactions that pay solely to P2WSH outputs?

yaslama commented 3 years ago

@Roasbeef we have cases where we broadcast using lnd api transactions with output given by the user, so we cannot make any assumptions about the output. @wpaulino in Breez, we launch neutrino from time to time as a background job, so the first solution is better for us. But the second solution is more than ok.

cjdelisle commented 3 years ago

I ended up fixing this for my own purposes via https://github.com/pkt-cash/pktd/blob/master/neutrino/query.go#L1551 which might be back-portable but I never pursued backporting any of the changes I made because, quite frankly, I changed a lot of design decisions and deleted a lot of code. But maybe someone will find some of it useful...

wpaulino commented 3 years ago

we have cases where we broadcast using lnd api transactions with output given by the user, so we cannot make any assumptions about the output. in Breez, we launch neutrino from time to time as a background job, so the first solution is better for us. But the second solution is more than ok.

@yaslama since all the transactions are sourced from lnd's wallet, launching neutrino as a standalone background process will not broadcast any transactions on its own.

I'll be working on addressing the continuous broadcast issue for neutrino lnd users (see my previous comment) some time throughout the lnd v0.14 cycle.

kingonly commented 3 years ago

Much appreciated @wpaulino 🙏

yaslama commented 3 years ago

I'll be working on addressing the continuous broadcast issue for neutrino lnd users (see my previous comment) some time throughout the lnd v0.14 cycle.

Thanks a lot @wpaulino

Roasbeef commented 3 years ago

I think it would be best for it to follow that approach, either by integrating the ChainNotifier or replicating its behavior.

Yeah I think the best option here is to have the rescan logic inherit some call back, so it can speak to rebroadcaster to let it know when something eventually gets confirmed or not.