Mock ingest transactions & produce blocks #325
Conversation
|
This PR is looking great so far! Glad you pulled out
Yes this is right.
So at this moment I am not entirely sure if we need the witness at all. @ben-chain let me know what you think, but it seems to me you should only need to send the aggregator a Is this what you were asking with this question @willmeister ? |
@karlfloersch the reason I added it is that we call |
|
I think that this is the right model, @karlfloersch we will probably end up calling transaction |
- BlockTransactionCommitment returned from ingestTransaction(...) - Ability to get Public Key of Aggregator to validate signed BlockTransaction - Some crypto utils - Range utilities Improves: - Tests for Aggregator, testing return type, range validity, and signature - Crypto verifySignature stub
… stuff copied and adapted from plasma-group#280 - Removed redundant constructor assigment of members - Adapted RangeBucket to fit BlockDB usage
…cks that are not safely concurrent
- Changing BlockDB to have buffer keys for BigNums - Adding serialization and deserialization for StateUpdate to serialize/parse objects to/from string
There was a problem hiding this comment.
Looks good on first pass, just a few comments.
Leaving to @karlfloersch @ben-chain to review
willmeister
changed the title
| * @returns a promise that resolves once the update has been added. | ||
| */ | ||
| public async addPendingStateUpdate(stateUpdate: StateUpdate): Promise<void> { | ||
| await this.blockMutex.runExclusive(async () => { |
| } | ||
| public constructor( | ||
| private stateDB: StateDB, | ||
| private pluginManager: PluginManager |
There was a problem hiding this comment.
I guess we will rename to deciderManager?
There was a problem hiding this comment.
Yeah, I think you're right. Does that make sense to do in a separate PR that OVM-ifies all of our outdated names?
There was a problem hiding this comment.
Yeah let's do in a separate PR
| export interface BlockTransactionCommitment { | ||
| blockTransaction: BlockTransaction | ||
| witness: any | ||
| } |
There was a problem hiding this comment.
It's great that you're mocking the tx commitments already!
| ingestTransaction( | ||
| transaction: Transaction, | ||
| witness: string | ||
| ): Promise<BlockTransactionCommitment> |
There was a problem hiding this comment.
It is unclear to me what the witness parameter does here. I believe it may have been part of our out of date spec.
Ideally we'd simply be sent the transaction itself. From this we run some authentication logic & generate a new StateUpdate. In fact this whole transaction ingestion process is under specified and so I just created an issue for it here: #349
There was a problem hiding this comment.
That's a good question. There is both authentication and authorization to be done, right?
- Is this transaction actually from the user who is trying to send their funds (authentication)?
- Does the user trying to send funds actually own the funds they're trying to send (authorization)?
I assumed the witness portion would solve (1), although that could be handled in the communication layer itself. Is there something I'm missing that either handles that or makes it unnecessary?
There was a problem hiding this comment.
I think that we can keep transactions more general and with that we won't need to break those two (authentication/authorization) into two steps. Instead you simple check that the transaction itself is valid. The tx might not even have a single sender--it could simply be a subsidized function call in a smart contract (essentially the concept of account abstraction ethereum/EIPs#859).
although that could be handled in the communication layer itself
Yeah I think it makes sense for the anti-spam mechanism to be handled by the communication layer. Eg. banning peers that repeatedly broadcast failing transactions.
There was a problem hiding this comment.
Thanks for the explanation 😄. I'll remove witness from the method signature.
…arate wallet for each async test instead of sharing
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
Adds Aggregator and BlockManager (copied from here) and implements and tests
Aggregator.ingestTransaction(...)Questions
enqueueStateUpdate(...)andproduceBlock()that are required?witnesstoAggregator.ingestTransaction(...)?Metadata
Fixes
Contributing Agreement