Mempool

[sekulicd]

This adds "MemPool" like service to the node. This is not real MemPool as it doesnt contain all logic like tx validation etc. Its function is to notify subscribers about 3 types of events:

• TxUnConfirmed - triggered on protocol tx msg
• TxConfirmed - every time protocol block msg arrives, MemPool checks if any of txs is is inside block, if yes it removes tx from the pool and publishes event
• TxRejected - if tx from the pool is not confirmed after "expiryTime" tx is removed from MemPool and TxRejected event it published, this is mechanism that prevents "flooding" MemPool with fake txs.

This closes #9

@tiero @altafan @louisinger please review.

[louisinger]

LGTM but after some reading this weekend about Bitcoin protocol I think we are on the wrong track. We use the tx message to receive transactions. But by definition, tx is the "response" to a getdata message. Empirically, I agree that we receive transactions via tx once broadcast (however, the node doesn't send getdata, which is weird...) it works but I'm afraid it's not very reliable in the future (because getdata is made for objects confirmed on the chain theoretically).

I discovered another message that seems to be made for this: mempool. It's an extended version of getdata that allows to retrieve unconfirmed transactions from a peer. https://en.bitcoin.it/wiki/Protocol_documentation#mempool

the mempool.go implem is OK in my opinion but I would add mempool msg support on nodeService to sync the mempool map instead of tx msg

[altafan]

I'll test this ASAP, but the tx rejection doesn't convince me: maybe this node should reject a transaction only when receiving a confirmed one that spends at least one of the inputs of the former.

Also, rejection is a nice-to-have feature but not a fundamental one because it can be handled client-side. For me, it's ok to eventually add it later, but for sure this kind of timeout-based rejection is wrong because the waiting time for a tx to be included in a block is not predictable.

[tiero]

Also, rejection is a nice-to-have feature but not a fundamental one

i do believe we should have a mechanism in place: instead of timeout/expiration based, I would simply have a size approach: ie. max 100MB of space, if we go over to that limit we start prune (ie. best term than reject) the old transactions starting with the oldest first (therefore we should have a "first seen" timestamp)

[sekulicd]

I discovered another message that seems to be made for this: mempool. It's an extended version of getdata that allows to retrieve unconfirmed transactions from a peer. https://en.bitcoin.it/wiki/Protocol_documentation#mempool

I had same doubt but i decided to use tx based on following: In wiki page there is "It is specified in BIP 35. Since BIP 37, if a bloom filter is loaded, only transactions matching the filter are replied." With BIP37 SPV client loads bloom filter to the full node with filterload msg, then with mempool msg full node return txs based on prev loaded filter. But since BIP37 has known issues(privacy, dos) BIP157&BIP158 defines other way in which full nodes generates and sends block filters to SPV clients so that they can do verification, which we are doing in our proj. My understanding is that we would need use BIP37 and load filter which as said is not good for privacy(there is question also on how we can trust full node about data it is sending). Also i was checking btcd code and what i found is that after tx is added to mempool it is "announcing" tx to its peers by sending inv msg in here.

Also, rejection is a nice-to-have feature but not a fundamental one ...

Originally, requirement was to have "UnConfirmed" notification, i was thinking it would be nice to have(at least for testing) other two event(Confirmed, Rejected).

[altafan]

i do believe we should have a mechanism in place: instead of timeout/expiration based, I would simply have a size approach: ie. max 100MB of space, if we go over to that limit we start prune (ie. best term than reject) the old transactions starting with the oldest first (therefore we should have a "first seen" timestamp)

This can be complementary to my proposal, keen to have a discussion on it, but what I'm trying to say is that since we are still not aligned on this, i wouldn't add code that it's likely to be changed in the future.

Originally, requirement was to have "UnConfirmed" notification, i was thinking it would be nice to have(at least for testing) other two event(Confirmed, Rejected).

We do already have notifications for confirmed transactions, what we need now is a way to get them also for unconfirmed ones indeed.

[tiero]

We do already have notifications for confirmed transactions, what we need now is a way to get them also for unconfirmed ones indeed.

Agreed! Let-s start with basic support for unconfirmed first (so lets drop Reject notification and expiration time for now), and handle spam/dos attacks (for not valid/censored txs filling the space on the neutrino node).

I do think would be nice tho to add in this PR a "first seen timestamp" field on each tx

[sekulicd]

We do already have notifications for confirmed transactions, what we need now is a way to get them also for unconfirmed ones indeed.

Agreed! Let-s start with basic support for unconfirmed first (so lets drop Reject notification and expiration time for now), and handle spam/dos attacks (for not valid/censored txs filling the space on the neutrino node).

I do think would be nice tho to add in this PR a "first seen timestamp" field on each tx

@tiero @altafan removed rejected logic.

[tiero]

TestSendTransaction seems to fail. Rather remove it if not used/unstable (or maybe nigiri do not expose the 18886 port, but seems strange)

EDIT: it seems is due siganture problem? https://github.com/vulpemventures/neutrino-elements/runs/6353024344?check_suite_focus=true#step:6:108