search

nimiq / core-rs

Official Rust implementation of the Nimiq protocol
https://nimiq.com
Other
74 stars 10 forks source link

Fixed panic in `Mempool::push_transaction` and head hash update in `Blockchain::rebranch` #13

Closed jgraef closed 5 years ago

jgraef commented 5 years ago

Pull request checklist

What's in this pull request?

On block 488308 a fork happened that was resolved at block 488309. After rebranching a transaction was pushed to the mempool which crashed the node. Somehow the sender balance was out of funds. It is possible for a transaction to be verified, but later when all transactions for the sender address are being rechecked, that the sender is out of funds. It is unclear if this caused the crash. The log looks like the transaction was received after the block was processed, but maybe the other transactions with the same sender address weren't removed from the mempool yet. I will check this later. I fixed this, by rejecting a transaction during rechecking if the sender is out of funds, as this should be the intended behavior. Another possible fix is to synchronize the whole Mempool::push_transaction over the Blockchain or Accounts.

After the crash the node didn't restart, because the rebranching code didn't update the head hash in the ChainStore, which caused the head's accounts tree hash to be incorrect. I fixed this by updating the ChainStore's head hash before commiting the rebranching transaction.

jgraef commented 5 years ago

Travis CI fails, because the code isn't updated to the latest version of Rust nightly.

jgraef commented 5 years ago

I just checked the balance at block 488308 and 488309:

So my hypothesis is that after the rebranch the transactions weren't cleared out of the mempool yet (is this possible if block processing finished?). When the new transaction is received the intrinsic check passes, because at block 488309 there is enough NIM in the balance. When rechecking all transactions of the sender, the transactions that were mined in block 488308 are also considered, thus the balance is -26215042 - the actual transaction value and fee. This would then cause the crash we saw.

paberr commented 5 years ago

Looks good to me!