-
Notifications
You must be signed in to change notification settings - Fork 30
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
[WIP] Added initial BlockManager and BlockDB implementations #280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, but LGTM 👍
// TODO: Figure out how to implement locking here so two state updates | ||
// can't be added at the same time. | ||
|
||
if (await block.has(start, end)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How frequently do we imagine Operators will submit plasma blocks? There's still a lot for me to learn, but this line triggers some red flags around scalability, depending on the answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably somewhere between 15sec and 10mins depending on the chain. Which part specifically raises red flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @kfichter, just seeing this now, but it means that the same range can only be transacted once every 10-15sec. That's probably fine for now, but I can certainly think of applications for which that will be too slow (trading, poker, etc.).
* Finalizes the next plasma block so that it can be published. | ||
* @returns a promise that resolves once the block has been finalized. | ||
*/ | ||
public async finalizeNextBlock(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that it matters, but there's a race condition here if called concurrently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I could definitely use some input on managing concurrency. Seems like we might have to manage it on a global/OS level because leveldb
isn't safe between different processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically important for the operator because accidentally bumping the block number twice would break a lot of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to look into this more. It could be as simple as using a lib that wraps some simple single-threaded bool-flipping logic. We should be able to use JS's single-threaded nature to enforce order where necessary on multiple awaited calls.
711640c
to
50df20b
Compare
… stuff copied and adapted from plasma-group#280 - Removed redundant constructor assigment of members - Adapted RangeBucket to fit BlockDB usage
Adds: - BlockTransactionCommitment returned from ingestTransaction(...) - Ability to get Public Key of Aggregator to validate signed BlockTransaction - Some crypto utils - Range utilities - Serialization and deserialization for StateUpdate to serialize/parse objects to/from string Adds BlockManager, Block DB CommitmentContract, and probably other stuff copied and adapted from #280 - Removed redundant constructor assigment of members - Adapted RangeBucket to fit BlockDB usage - Adding async-mutex lib and adding mutual exclusion around BlockDB blocks that are not safely concurrent - Changing BlockDB to have buffer keys for BigNums Fixes: * Wallet nonce race condition in Deposit tests by creating a separate wallet for each async test instead of sharing * Increasing default test timeout from 2 seconds to 5 seconds to fix CI build failures
Adds: - BlockTransactionCommitment returned from ingestTransaction(...) - Ability to get Public Key of Aggregator to validate signed BlockTransaction - Some crypto utils - Range utilities - Serialization and deserialization for StateUpdate to serialize/parse objects to/from string Adds BlockManager, Block DB CommitmentContract, and probably other stuff copied and adapted from plasma-group#280 - Removed redundant constructor assigment of members - Adapted RangeBucket to fit BlockDB usage - Adding async-mutex lib and adding mutual exclusion around BlockDB blocks that are not safely concurrent - Changing BlockDB to have buffer keys for BigNums Fixes: * Wallet nonce race condition in Deposit tests by creating a separate wallet for each async test instead of sharing * Increasing default test timeout from 2 seconds to 5 seconds to fix CI build failures
Description
Started implementing BlockManager and BlockDB. Still WIP but want to get review as I go along. Partially blocked by #271 at the moment because
RangeStore
is currently located inOperator
but I don't want to do an import when we're moving it soon anyway.Questions
BlockDB
needs to lock a specific range so that a race condition doesn't cause a database overwrite. Can this be handled at theLevelDB
level? How important is this?BlockManager
maintain an append-only log like it did back in the original operator?BlockManager
be writing toStateManager
orHistoryManager
once it publishes a block? It doesn't really make sense for bothBlockManager
andHistoryManager
to store the full set of state updates (?).Metadata
Fixes
Blockers
Contributing Agreement