New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically remove stale pending transactions #463

Merged
merged 2 commits into from Mar 16, 2017

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented Mar 14, 2017

During a recent support issue it became evident that it is possible for a pending transaction to become stale (eg if orphaned) and thus never removed from the PendingMap. This causes the balance provided at the RPC layer to be incorrect until the issue is discovered and the node restarted (the PendingMap is not persisted).

This PR adds an automatic check which removes transactions from the PendingMap if they have been removed from the mempool.

The change can be demonstrated using regtest. First we'll send a transaction to create a pending entry:

$ omni_send mpGRoxX1pQwGWfxEzMDjZ9ihx6kh3eErju mpv4tJcAcSFodJJfM5DkWxNAKgNRdrdtH8 1 10
602f381ff5fb0a9fde3624fcabe7c53b058911234509480ac3f06285777720c4
2017-03-14 06:49:42 PendingAdd(602f381ff5fb0a9fde3624fcabe7c53b058911234509480ac3f06285777720c4,mpGRoxX1pQwGWfxEzMDjZ9ihx6kh3eErju,0,1,1000000000,true)
2017-03-14 06:49:42 update_tally_map(mpGRoxX1pQwGWfxEzMDjZ9ihx6kh3eErju, 1=0x1, -1000000000, ttype=3): before=0, after=-1000000000

We can check that the transaction is now pending:

$ omni_listpendingtransactions
[
  {
    "txid": "602f381ff5fb0a9fde3624fcabe7c53b058911234509480ac3f06285777720c4",
    "fee": "0.00008100",
    "sendingaddress": "mpGRoxX1pQwGWfxEzMDjZ9ihx6kh3eErju",
    "referenceaddress": "mpv4tJcAcSFodJJfM5DkWxNAKgNRdrdtH8",
    "ismine": true,
    "version": 0,
    "type_int": 0,
    "type": "Simple Send",
    "propertyid": 1,
    "divisible": true,
    "amount": "10.00000000",
    "confirmations": 0
  }
]

Now let's empty the mempool:

clearmempool
[
  "602f381ff5fb0a9fde3624fcabe7c53b058911234509480ac3f06285777720c4"
]

We can then check that the transaction is no longer pending:

omni_listpendingtransactions
[
]

Finally we can mine a block, and confirm that the missing pending transaction has been detected, deleted and the amount credited back to the tally:

generate 1
[
  "4e12052669793f3f06b6bd618516c3f739295ca2933f67440296ee7462ca9095"
]
2017-03-14 06:52:24 WARNING: Pending transaction 602f381ff5fb0a9fde3624fcabe7c53b058911234509480ac3f06285777720c4 is no longer in this nodes mempool and will be discarded
2017-03-14 06:52:24 PendingDelete(602f381ff5fb0a9fde3624fcabe7c53b058911234509480ac3f06285777720c4): amount=-1000000000
2017-03-14 06:52:24 update_tally_map(mpGRoxX1pQwGWfxEzMDjZ9ihx6kh3eErju, 1=0x1, +1000000000, ttype=3): before=-1000000000, after=0

Thanks
Z

@dexX7 dexX7 added this to the Next release milestone Mar 14, 2017

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Mar 14, 2017

Looks good to me! Ready to go? :)

@dexX7 dexX7 removed the status: ready label Mar 14, 2017

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Mar 14, 2017

The mempool changes all the time, so I'm not sure, if it's sufficient, if we only check, whether the transactions are still in the pool, when a new block is mined?

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Mar 14, 2017

Since it only affects the node that sent the transaction in the first place I figured it would be sufficient - the aim was to avoid a pending subtraction becoming "semi-permanent" until a node restart.

We need some kind of trigger to make the check - it's possible to hook into the Bitcoin Core side of things and do the PendingMap deletion when the underlying Bitcoin code removes a transaction from the mempool but I thought that approach would be a bit more intrusive.

@dexX7 dexX7 added the status: ready label Mar 16, 2017

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Mar 16, 2017

We need some kind of trigger to make the check - it's possible to hook into the Bitcoin Core side of things

Yeah, at some point in the future we could hook the wallet/transaction signals.

@dexX7 dexX7 merged commit 162d505 into OmniLayer:develop Mar 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

dexX7 added a commit that referenced this pull request Mar 16, 2017

Merge #463: Automatically remove stale pending transactions
162d505 Enable the pending mempool check in the block_end handler (Zathras)
9d1e400 Add check to verify pending transactions are still in the mempool (Zathras)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment