search

bcoin-org / bcoin

Javascript bitcoin library for node.js and browsers
https://bcoin.io
Other
2.99k stars 814 forks source link

In SPV mode, broadcast transactions are not always added to wallet. #631

Open xloem opened 5 years ago

xloem commented 5 years ago

Although it works fine when running as a full node, I can't seem to get my bclient wallet to acknowledge manually broadcast transactions, especially if I craft them without calling any wallet functions.

It often seems unaware of them until the block is confirmed, which makes it very easy to accidentally doublespend, as the wallet still believes the coins are spendable.

This would be easy to create a workaround for if WalletClient provided a function to manually add a transaction, but I don't see any such function in the docs.

pinheadmz commented 5 years ago

Could you elaborate a bit on this? I might've had similar experience but not sure how to reproduce. When I send a tx with bwallet, it shows up in my history. Is it not emitting tx events for you or something?

xloem commented 5 years ago

@pinheadmz, running spv, I am not getting any bclient wallet events, nor do my broadcast transactions show up in my history until they are confirmed. These are valid transactions that are manually crafted. I uploaded https://github.com/xloem/bclientstore -- test.js shows it happening briefly.

braydonf commented 5 years ago

During testing, I have noticed something similar with missing (and delayed) transactions being propagated to an spv node from a full node.

braydonf commented 5 years ago

After looking more into my test case, the transactions were added; however, there was a delay because await wclient.execute('sendtoaddress', [addr, amount]) does not wait for the transactions to be broadcast once, only that it has been added to the mempool (it's then separately broadcast to other nodes after that).

Edit: Scratch that, there do indeed seem to be transactions missing still.

braydonf commented 5 years ago

Not sure if related to the original issue. However my issue was caused by too-long-mempool-chain of txs, which was causing issues, see below:

[error] (node) Error: Verification failure: too-long-mempool-chain (code=nonstandard score=0 hash=6e8d76f56909fff0d7a2d5a7312495250a7090fb3f3a1c10ce33cdc4dec070bf)
   at Mempool.verify (./bcoin/bcoin/lib/mempool/mempool.js:971:13)
   at <anonymous>

After that error there were a bunch of these:

[warning] (node) TX was orphaned in mempool: 3307350433040a8a045836deb2e61ac787f93af96e490339749047bd55df5d7d.
[warning] (node) Attempting to broadcast anyway...

And many likely didn't get broadcasted.

I reduced the number of broadcasted transactions in the testcase for the time being to resolve it.

nodech commented 5 years ago

@xloem So if I understand correctly, you don't want to use wallets functionality to construct transactions, that happens when you are doing manual construction.

Node and Wallet are separate in bcoin. SPV does not have mempool (where fullnode double checks if wallet has transaction or not -- also mempool verifies tx is okay), when you broadcast transaction using node.broadcast in SPV mode it's not sent to the wallet, it will go to the network and wallet wont be notified.

You might expect that you will receive that transaction from your nodes, but unfortunately nodes keep track who sent the information to minimize network traffic, so none of your existing peers will send it back to you.

xloem commented 5 years ago

@nodar-chkuaselidze yes, exactly. I'm sending OP_RETURN transactions.

The result of the behavior you describe is that in fullnode mode, manual transactions are immediately reflected in the wallet, whereas they result in corrupt (outdated) wallet state in SPV mode, that can result in double-spends and unavailable balance especially if there are multiple bclient apps. Is this intended behavior?

braydonf commented 5 years ago

I have noticed another issue with transactions being sent to SPV nodes from peers in the case that there are more than 10 or 20 transactions broadcast quickly. The issue is especially heinous with the CPU of the SPV node is loaded or limited.

This is most likely due to the 10 transaction lookahead that needs to be updated, with keys being derived, and the new addresses added to the bloom filter and sent to neighboring peers. Without that bloom filter update, neighboring peers won't send the transaction.

So it's a race and the transactions are lost if the bloom filter isn't updated in time. I have a reproducible case in https://github.com/bcoin-org/bcoin/pull/605 and have modified the lookahead in lib/wallet/account.js during testing and it has has changed the behavior.

itsMikeLowrey commented 4 years ago

Is there any way to work around the issues of spv nodes broadcasting transactions, but the wallet not getting notified? I am currently using an api to broadcast to avoid this issue, but I would love to have a more decentralized solution. Thanks for the help.

pinheadmz commented 4 years ago

@itsMikeLowrey could you expand on your use case a bit? Are you using bcoin's wallet to generate the transactions? What is your setup? We can also chat on IRC freenode #bcoin or slack: https://bcoin.io/slack-signup

pinheadmz commented 4 years ago

Is this PR by @OrfeasLitos not a solution to this unexpected behavior? https://github.com/bcoin-org/bcoin/pull/417

OrfeasLitos commented 4 years ago

Let me know if you plan to merge #417, in that case I'll fix conflicts.