Save pending local transactions in the database #4566
Conversation
rphmeier
added
A0-pleasereview 🤓
rphmeier
changed the title
parity/run.rs
Outdated
| @@ -314,6 +329,31 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger>) -> R | |||
| let client = service.client(); | |||
| let snapshot_service = service.snapshot_service(); | |||
|
|
|||
| // initialize the local node information store. | |||
| // TODO: tick it every once in a while? | |||
There was a problem hiding this comment.
in the case of a panic or other dirty exit where destructors aren't run, the list of transactions in the database won't be updated. we might want to tick periodically (once every 5 minutes?) to ensure a higher chance of transactions making it in.
local-store/src/lib.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| #[should_panic] |
There was a problem hiding this comment.
I thought that it shouldn't panic?
There was a problem hiding this comment.
It'll panic because of the assertion that the loaded set is equal to the stored. They aren't equal because loading skips all transactions with an invalid signature.
There was a problem hiding this comment.
So maybe it would be better to have assert(expected != actual, "They aren't equal because loading skips all transactions with an invalid signature.") instead? Should panic means that it can actually panic anywhere - imho it's suitable only for simple function calls not tests like this.
| pub struct ToV11(Progress); | ||
|
|
||
|
|
||
| impl Migration for ToV11 { |
There was a problem hiding this comment.
Is this migration necessary? I thought that you an just append new columns without copying the whole database.
On-the-fly adding new Column Families and dropping them. Both operations are reasonably fast.
https://github.com/facebook/rocksdb/wiki/Column-Families#introduction
There was a problem hiding this comment.
Right, the existing behavior is an artifact of our migration system, which was written before we had column families. We can probably add an in-place upgrade mode, but we'd still have to create a backup copy.
Not sure if that's within the scope of the PR.
There was a problem hiding this comment.
But you can just get rid of the migration at all, right? (just alter v10 to return more columns and that's it) Since if I understand correctly opening a database specifying more columns should work fine.
There was a problem hiding this comment.
AFAIK you have to open the database with N columns and then add the N + 1th. Our migration manager doesn't support this operation currently. It should be simple enough to add, though. Technically, the operation of adding a column family can fail, so we still have to back up a copy of the pre-migrated database to fall back on.
|
Now updates the store once every 15 minutes. |
| @@ -212,7 +214,7 @@ impl Writable for DBTransaction { | |||
| } | |||
| } | |||
|
|
|||
| impl Readable for Database { | |||
There was a problem hiding this comment.
KeyValueDB and KVDB? really no better naming than this?
There was a problem hiding this comment.
ideally we'd have the trait called kvdb::Database, but our rocksdb wrapper has already taken that name and I didn't want to break all existing usages of it (although the trait name KeyValueDB is arguably more descriptive). I could use a single-letter name for the generic param, but I don't think it really matters.
gavofyork
added
A8-looksgood 🦄
rphmeier
added
the
B7-releasenotes 📜
Now you can schedule transactions, restart your node, and still have them propagate correctly!
Adds a "node info" column family which will be used for general local node information (in the future, sync security level and anything else we might come up with). Introduces a migration for this purpose, solidifying the implicit incompatibility between 1.5 and 1.6-era DBs which already exists due to changes in receipt format.
All non-mined (pending and future) local transactions stored, along with their conditions and restored upon startup.
Also abstracts over the
Databaseusing aKeyValueDBtrait implemented for an in-memory store as well, so we no longer require I/O for most DB tests.Part of #1966
Fixes #2336