Problem (Fix #1069): blocks with multiple txs may fail light client app hash check #1117
Conversation
yihuang
requested review from
calvinaco,
devashishdxt and
tomtau
as code owners
Yeah, I just want to unblock @calvinlauco 's work first, we can add the integration tests like discussed on slack. |
Thanks @yihuang , Let me try with this first and get back to you. |
|
Fixed the unit test build. |
Tested, it passed the problematic block :D |
Codecov Report
@@ Coverage Diff @@
## master #1117 +/- ##
==========================================
+ Coverage 64.37% 64.67% +0.29%
==========================================
Files 148 148
Lines 19393 19495 +102
==========================================
+ Hits 12485 12608 +123
+ Misses 6908 6887 -21
|
i think there could at least be a small unit test that captures the bug (it may need isolating out some of the sync functionality) |
@yihuang and then I applied your fix here |
|
I just added a multinode integration test for this and cherry-picked @calvinaco 's unit test. |
|
This pull request introduces 1 alert when merging 53bdae3 into 0bbfbbf - view on LGTM.com new alerts:
|
yihuang
force-pushed
the
issue1069
branch
2 times, most recently
from
05d821d to
a541d6b
Compare
Done |
|
bors r+ |
1116: Problem: uncommitted chain-abci storage changes may persisted (fixes #885) r=tomtau a=tomtau Solution: for the TX-related ops, added a temporary map in storage: - insertions during delivertx will update the temporary map + insert entry into the current db tx - at commit, the db tx is written and flushed - lookup extended to indicate whether the caller wants to read the uncommitted change (as in delivertx -- e.g. if two transactions in the same block want to spend the same utxo, only the first one is valid, so processing needs to read the uncommitted change from the first one before processing the second one) or committed change (e.g. in queries) added two quick unit tests (the first one used to fail with "buffered_write" -- "buffered_write" was removed, as inserts are kind of buffered writes) 1117: Problem (Fix #1069): blocks with multiple txs may fail light client app hash check r=tomtau a=yihuang Solution: - Change BTreeMap to IndexMap Co-authored-by: Tomas Tauber <[email protected]> Co-authored-by: yihuang <[email protected]>
| @@ -56,11 +56,11 @@ pub struct Attribute { | |||
|
|
|||
| impl BlockResults { | |||
| /// Returns transaction ids and the corresponding fees in block results | |||
| pub fn fees(&self) -> Result<BTreeMap<TxId, Fee>> { | |||
| pub fn fees(&self) -> Result<IndexMap<TxId, Fee>> { | |||
There was a problem hiding this comment.
could you add some explanation why "change to IndexMap"?
Build failed (retrying...) |
| last_height = latest_block_height(rpc) | ||
|
|
||
| print('Send multiple tx') | ||
| pending_txs = [rpc.wallet.send(transfer1, amount) for _ in addresses] |
There was a problem hiding this comment.
is rpc.wallet.send async?
It seems there needs to be some "wait", otherwise node1 may be started up before everything else?
There was a problem hiding this comment.
is
rpc.wallet.sendasync?
It seems there needs to be some "wait", otherwise node1 may be started up before everything else?
Even if it's async, the tx should be processed eventually? Maybe wait_for_tx need some more timeout?
There was a problem hiding this comment.
Changed a little bit, let's see how that works?
There was a problem hiding this comment.
Even if it's async, the tx should be processed eventually?
yes, but my guess is that the goal is here is that these TXs end up in mempool while node1 is down, so that they all get picked up for the same block. If node1 starts up before that / while they are being submitted, they may get picked up immediately for different blocks
There was a problem hiding this comment.
Even if it's async, the tx should be processed eventually?
yes, but my guess is that the goal is here is that these TXs end up in mempool while node1 is down, so that they all get picked up for the same block. If node1 starts up before that / while they are being submitted, they may get picked up immediately for different blocks
Yes, I added 1-second sleep for this.
1117: Problem (Fix #1069): blocks with multiple txs may fail light client app hash check r=tomtau a=yihuang Solution: - Change BTreeMap to IndexMap Co-authored-by: yihuang <[email protected]>
…t client app hash check Solution: - Change BTreeMap to IndexMap :white_check_mark: Add test case to cover mismatch app hash
Canceled |
|
bors r+ |
Build succeeded |
Solution: