ABCI chain migration conclusion#2488
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2488 +/- ##
==========================================
+ Coverage 91.48% 91.73% +0.24%
==========================================
Files 41 41
Lines 2396 2467 +71
==========================================
+ Hits 2192 2263 +71
Misses 204 204 |
bigchaindb/core.py
Outdated
| 'ABCI request - you need to migrate the ABCI client ' + \ | ||
| 'and set new chain ID: {chain_id}.' | ||
| logger.error(msg) | ||
| return ResponseInitChain() |
There was a problem hiding this comment.
Why is init_chain returning a successful response even when the system is receiving invalid parameters?
The above query applies to all other places where an error log is generated but the return value is a success response.
There was a problem hiding this comment.
No error codes are documented for InitChain.
There was a problem hiding this comment.
Since we cannot send error codes back we need to panic, just like Tendermint does when it sees a height greater than what it has witnessed.
There was a problem hiding this comment.
Which goal do we achieve by causing panic?
If there is a process manager, the process will be restarted and it will start all over.
There was a problem hiding this comment.
The reason I am suggesting to panic is because the system is starting with invalid information. There is not point sending success response when we can't do any further processing.
There was a problem hiding this comment.
Tendermint state is changed somehow if we return a response, what is not the case if we panic, so you have point.
There was a problem hiding this comment.
I am in the process of updating other ABCI handlers - it probably makes sense to abort everywhere until the latest chain is in sync, right?
ldmberman
force-pushed
the
abci-chain-migration-conclusion
branch
from
a6d73c6 to
b92139f
Compare
bigchaindb/core.py
Outdated
| """ | ||
|
|
||
| chain = self.bigchaindb.get_latest_abci_chain() | ||
| self.abort_if_abci_chain_is_not_synced(chain) |
There was a problem hiding this comment.
As discussed in our office conversation it would be better to load the genesis for the fork during __init__() and then use it inside info, init_chain and check_tx
| connection.conn[dbname].utxos.delete_many({}) | ||
| connection.conn[dbname].validators.delete_many({}) | ||
| for t in TABLES: | ||
| getattr(connection.conn[dbname], t).delete_many({}) |
| } | ||
|
|
||
|
|
||
| def test_create_secondary_indexes(): |
There was a problem hiding this comment.
The indexes on abci_chains collection should also be asserted
ldmberman
force-pushed
the
abci-chain-migration-conclusion
branch
from
40fa6fa to
5b7e9fe
Compare
|
Regarding the failed tests, see #2507. |
Solution: Make get_validator_set/get_validators return None/[] when there are no validators yet.
Solution: Record known chains and sync through InitChain. Triggering the migration and adjusting other ABCI endpoints will follow.
It generates and records a chain ID.
And abort if the current chain is not synced.
ldmberman
force-pushed
the
abci-chain-migration-conclusion
branch
from
5b7e9fe to
314c7ea
Compare
3 participants
Addresses the main point from #2468.