nopara73 / HBitcoin

Privacy focused Bitcoin library on top of NBitcoin for .NET Core.
MIT License
25 stars 11 forks source link

Add WalletState -> SyncingMempool? #10

Closed nopara73 closed 7 years ago

nopara73 commented 7 years ago

After: https://github.com/nopara73/HBitcoin/issues/6 Related: https://github.com/nopara73/HBitcoin/issues/11

Right now the Wallet have 3 states:
Doing nothing, syncing and synced.

Probably it'd be a better idea to have: 4:
doing nothing,
syncing blocks,
syncing mempool,
synced

dangershony commented 7 years ago

If you look at the pooler, blocks and trx are all the time syncing so the wallet doesn't need to be in a state, the tracker just needs to handle new blocks and new trx (trx concurrently) and manage reorg when a new block is pushed which is not referencing prev.

nopara73 commented 7 years ago

I'll answer to this other question here, since they are related. At that issue the question was what my mempoool, more specifically MemPoolJob does:

Until blocks are synced mempool level transactions should not be coming, they should not use bandwidth at that point. This is what the "StallMempool" issue (https://github.com/nopara73/HBitcoin/issues/11) is created for. I implement this logic, but I lose 1-2 seconds at: between the last block is downloaded and the mempool starts syncing.
So what does it do?

  1. Until blocks are fully synced: nothing.
  2. Keeping an up to date list of all transaction hashes the nodes I am connected to have in their mempool.
  3. Raising an event when a new transaction arrives (not only its hash).

I don't see how can you avoid having "syncing mempool state", since you have to initially download/go through all the transactions from the mempool and that's not small data.
In fact there are 3 mempool specific states I defined in the ApiSpecification:

The mempoolStalling state unrelated to issue https://github.com/nopara73/HBitcoin/issues/11 , in that issue the mempool is stalling, because headers are changing, however the mempoolStalling state in the ApiSpecification is API user indicated mempool stopping with the GET /wallet/mempool/?allow=[true/false] - Allows or disallows mempool syncing. In fact it might be better to call it mempoolStopped or something like this, anyway this is not implemented yet, but not big deal.

The rest 2 states are evident.

dangershony commented 7 years ago

ah ops a typo I meant the block poller.

I don't think you need to sync mempool's from peers, this is heavy and unnecessary. if anything then we only need to do that once on start. once we are connected to nodes we will start to listen for broadcast transactions from peers (this is done using a NodeBehavior and a TransactionSignaled object), when a trx is received we immediately check if it's "ours" then keep it and notify the GUI of a pending trx. if its not ours we just discard (or keep the hash so we don't check again).

Then when a block is received the trx is marked as confirmed

I think actually as oppose to a full node we need to listen to trx even if the node is not synced as you might be syncing then also receive a trx to your wallet

Its very likely you open the wallet expecting a trx but the node is still catching up.

nopara73 commented 7 years ago

What?

I don't think you need to sync mempool's from peers, this is heavy and unnecessary.
&& I think actually as oppose to a full node we need to listen to trx even if the node is not synced as you might be syncing then also receive a trx to your wallet Its very likely you open the wallet expecting a trx but the node is still catching up.

This is a contradiction, if you don't sync the full mempool you'll not see any transaction that has been propagated before the user opened the wallet and blocks are not confirmed.

When?

if anything then we only need to do that once on start....

Yes, this would resolve that. The question is when?

  1. Before the blocks are synced.
  2. After the blocks are synced.

You proposed (1) before the blocks are synced, what can lead to unexpected behaviours and slow down the block syncing, too, as you put it "this is heavy". Basically I aim to follow an order, which reflects the states and with that make sure things doesn't go wrong: not started -> header download -> block download -> mempool stopped -> mempool syncing -> mempool synced
You can always hack and break things later, so I'd propose to keep the order of the things and go with the (2) after the blocks started syncing.

Missing

There is one thing that's missing from your proposed scheme and I don't see how to resolve, you might do.

I wan't an up-to-date list of mempool transactions. Right now I only store the hashes for performance, but I could easily sotore all transactions if I'd wanted to. I will most likely need this later, however I need it today at only one place.

The Tracker is tracking mempool transactions too. If the tx malleated or never confirmed (fall out of mempool) then it should not be tracked anymore. Right now this is how I solve this:

foreach(var tx in trackedMemPoolTransactions)
{
    // If we are tracking a tx that is malleated or fall out of mempool (too long to confirm) then stop tracking
    if(!MemPoolJob.Transactions.Contains(tx.GetHash()))
    {
        Tracker.TrackedTransactions.TryRemove(tx);
        Debug.WriteLine($"Transaction fall out of MemPool: {tx.GetHash()}");
    }
}

As you see I use the up-to-date transaction hashes of the mempool of the connected nodes.
One sloppy solution would be to do this check at the initial mempool syncing, however
(1) it is sloppy, what if for example someone runs our software forever? It'll never detect the above described falling out transactions?
(2) it will not have an updated list of transactions in the mempool and therefore limiting my future possibilites, possibly TumbleBit integration.

dangershony commented 7 years ago

You'r current approach of having a mirrored mempool of peers and continuously sync with peers to keep up to date is not what I had in mind. If we ask peers to relay trx then we don't need to ask the mempool (only once at start) after that we will keep getting notifications when new trx appear on the network.

When a transaction "falls out" its up to each node to decide if it doesn't want to keep that trx in memory anymore, to be honest once a trx not form the nodes wallet is relayed and the node is not a mining node I don't see why it needs to keep transactions (maybe for compact blocks or inv requests).

nopara73 commented 7 years ago

https://github.com/nopara73/HBitcoin/commit/987dfc6dc753708ee8e249bb4d44f039713d7fa6