Add omni_discardmempooltransaction call

[zathras-crypto]

This PR serves to add the capability to Omni Core to manually remove a transaction from the mempool.

Talking with @achamely on issues with OmniWallet whereby a transaction is sent with too low a fee and doesn't get included in a block then gets stuck in the mempool (after being discarded by the rest of the network), Creating a second transaction to respend those inputs with a higher fee does not work as it's essentially trying to broadcast a double spend through the same node which will be rejected. A restart of the node is then required (or broadcasting through an alternate node).

A new call is thus added with this PR; omni_discardmempooltransaction, just supply a TXID.

In using this call, the transaction will be discarded from the mempool, but will be readded if another node relays it back so the workflow should be to prepare the replacement transaction, remove the stuck transaction from the mempool and immediately broadcast its replacement.

Note: this PR has nothing to do with RBF.

Note: it's important to note that ideally this shouldn't be used on wallet transactions (the command does not remove transactions from the wallet, just the mempool - since OmniWallet doesn't use the wallet functions of Omni Core this shouldn't be an issue).

Usage is pretty simple. If we look at my current mempool (at time of writing with a just-started node):

zathras@OMNIDEV:~/github/build/omnicore$ src/omnicore-cli getrawmempool
[
    "4674f652165d022b60c64ac13bfe152acfe149521ddbfd2e7892515a209f6ad1",
    "666c91bc4d24a4e323239f9a97bcea8ec4c056678c7cb6f4c5f0dbddb1d74ac0",
    "93b4f2d8573f613be0b0b905a079555a6adf2a676d8babb8aee7a3b5b9aacc5a",
    "a5e172aa69a45be37cd2ff1905f81226bfbeb57aced6ce44c3dbb55ec9ce8cde",
    "ba9d5158baddca54e8a7ea17c19eb0d34c5d3020c92e0ebb2be3c0788c717752",
    "c6c0ad63eab139fa86eba7735e213d1a94fc9c5c45c7ee4cd39266ab703e202f",
    "ec7194b411944fa2771da55103e826185599bd99377dcb06e2cdfc5fec5a2626"
]

Let's remove the first transaction from the mempool:

zathras@OMNIDEV:~/github/build/omnicore$ src/omnicore-cli omni_discardmempooltransaction "4674f652165d022b60c64ac13bfe152acfe149521ddbfd2e7892515a209f6ad1"
[
    "4674f652165d022b60c64ac13bfe152acfe149521ddbfd2e7892515a209f6ad1"
]

Now if we check the mempool again, we can see the transaction we removed is no longer there:

zathras@OMNIDEV:~/github/build/omnicore$ src/omnicore-cli getrawmempool
[
    "666c91bc4d24a4e323239f9a97bcea8ec4c056678c7cb6f4c5f0dbddb1d74ac0",
    "93b4f2d8573f613be0b0b905a079555a6adf2a676d8babb8aee7a3b5b9aacc5a",
    "a5e172aa69a45be37cd2ff1905f81226bfbeb57aced6ce44c3dbb55ec9ce8cde",
    "ba9d5158baddca54e8a7ea17c19eb0d34c5d3020c92e0ebb2be3c0788c717752",
    "c6c0ad63eab139fa86eba7735e213d1a94fc9c5c45c7ee4cd39266ab703e202f",
    "ec7194b411944fa2771da55103e826185599bd99377dcb06e2cdfc5fec5a2626"
]

@achamely - does this do the trick?

Thanks
Z

[dexX7]

I'm sort of doubtful this actually works, because I'd assume connected peers won't accept the updated transaction, if they already have them?

Other than that, before Omni Wallet implements a fancy way to replace transactions, it would be a better start to use proper fee estimations.

[achamely]

@zathras-crypto @dexX7
Yes Omni will use a better fee setup first. However eventually there may be a need for this functionality. Z if it works as described then i think it'll be fine however Dexx does raise an interesting point and it may be something to consider if other nodes won't accept the new tx where does that leave our node once it accepts the new tx.

[zathras-crypto]

@dexX7 agreed fee-handling is important, and I've talked to the OW guys about using estimatefee to better determine fee - it's in the pipeline I believe but pending the rebrand PR.

I understand what you're saying about peers accepting the updated transaction, but please consider the use case:

From @achamely :

[3/11/2016 9:24:21 AM] Adam Chamely: user had 217 utxo's on a paper wallet address
[3/11/2016 9:24:40 AM] Adam Chamely: he imported the address into omniwallet, and created a send to move funds to multisig address
[3/11/2016 9:25:11 AM] Adam Chamely: he left the default fee at .0001 ( :( really need to get that fixed soon) and the transaction, expectedly was eventaully removed from network

And my introduction to the PR :

...issues with OmniWallet whereby a transaction is sent with too low a fee and doesn't get included in a block then gets stuck in the mempool (after being discarded by the rest of the network)...

Long story short it's specifically intended for use when the rest of the network has discarded the transaction. My understanding of the issue from @achamely is that his local node (the one he broadcast through) did not discard the transaction whilst the rest of the network did, and this PR is simply to allow that edge case to be cleaned up without a restart of the node.

Hope that helps clarify :)

[dexX7]

Long story short it's specifically intended for use when the rest of the network has discarded the transaction.

My bad, I missed that part.

Well, I'm still suspicious, but if you guys think this will work, I'm absolutely fine with it. :)

Code looks good to me.

[dexX7]

@achamely: if you need a quick fix immediently, you may try the hidden RPC clearmempool, which clears the whole mempool.

[zathras-crypto]

if you need a quick fix immediently, you may try the hidden RPC clearmempool, which clears the whole mempool.

[Hangs head in shame]

Sorry guys, looks like I wasted all our time! I did look for an existing call to remove a transaction from the mempool and didn't find one, hence this PR. Given that there is a call to clear the mempool already, this PR is kind of pointless.

@achamely since you're not mining blocks you can simply flush the mempool in its entirety to achieve the same thing this PR does.

This PR is on a more granular level, but unless you're using the mempool for other purposes ('Chest uses the mempool to provide unconfirmed tx support) then may as well just flush it empty.

Should I close this without merging? I don't see much value in adding capability that's essentially already there.

Thanks
Z

[dexX7]

Hey Z, don't leave your head hanging. :) 'clearmempool' is the last resort in my opinion, and in case of Omni Wallet the finer level may be needed, to avoid messing with outgoing transactions. Then again, these may already be in other node's mempools.

As mentioned, I'm fine with the PR, if Adam needs it. :)

[achamely]

Thanks guys, its not any immediate necessity (the issue that caused the original problem i was able to work around). But do keep it open for now we may want to revisit it in another week or 2 after we finish the rebrand and start working on the newer enhanced features.

[zathras-crypto]

No worries, we'll put it on hold for now :)

[dexX7]

Given this PR is outdated and needs further review, it's going to be closed for now.