-
Notifications
You must be signed in to change notification settings - Fork 284
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
feat: tx verification & peer scoring on p2p layer #8298
Conversation
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
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.
LGTM! Just a nit on managing dependencies, feel free to ignore if you disagree.
new GlobalVariableBuilder(config), | ||
new TestCircuitVerifier(), | ||
worldStateSynchronizer, |
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.
Rather than passing all the deps for the TxValidators across the p2p client and lib p2p service, and creating a new TxValidator for every tx, I'd change it so this call accepts a TxValidator (or a TxValidatorFactory if needed) directly. So when we add new validations with new deps we don't need to keep changing the entire call chain.
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.
I started writing it this way actually!
Issue is we sort of need to know which specific validation fails or not in order to then update peer scores based on the result
yarn-project/p2p/package.json
Outdated
@@ -60,6 +60,7 @@ | |||
"@aztec/circuits.js": "workspace:^", | |||
"@aztec/foundation": "workspace:^", | |||
"@aztec/kv-store": "workspace:^", | |||
"@aztec/simulator": "workspace:^", |
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.
What does it need the simulator for?
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.
WorldStateDB
is required for the double spend validator
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.
Hmm, no it needs a NullifierSource
which can be built in a much more lightweight fashion.
NullifierSource
defines a single function getNullifierIndex
which can be implemented by simply calling findLeafIndex
on an instance of MerkleTreeOperations. No need to pull in the complete simulator here.
private async isValidTx(tx: Tx): Promise<boolean> { | ||
const blockNumber = (await this.l2BlockSource.getBlockNumber()) + 1; | ||
|
||
const newGlobalVariables = await this.globalVariableBuilder.buildGlobalVariables( |
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.
Oof, this is an expensive call to be doing on every transaction. It makes lots of calls to the contract. We already have the block number and we just need the chain id, which never changes. Can you update the MetadataTxValidator to just take the data it needs and get rid of this global variable stuff?
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.
Just a couple of things
@@ -229,7 +247,16 @@ export class LibP2PService implements P2PService { | |||
heartbeatInterval: 1_000, | |||
mcacheLength: 5, | |||
mcacheGossip: 3, |
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.
As a separate PR can we get all of these gossipsub params into config?
}); | ||
const [___, doubleSpendInvalidTxs] = await doubleSpendValidator.validateTxs([tx]); | ||
if (doubleSpendInvalidTxs.length > 0) { | ||
// check if nullifier is older than 20 blocks |
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.
Can this go in config too?
[PeerErrorSeverity.HighToleranceError]: 50, | ||
}; | ||
|
||
export class PeerScoring { |
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.
I feel like we should have some unit tests around this class.
scoreParams: createPeerScoreParams({ | ||
topics: { | ||
[Tx.p2pTopic]: createTopicScoreParams({ | ||
topicWeight: 1, |
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.
Should these be in config?
} | ||
|
||
export const PeerPenalty = { | ||
[PeerErrorSeverity.LowToleranceError]: 2, |
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.
Should these values be configurable?
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.55.1</summary> ## [0.55.1](aztec-package-v0.55.0...aztec-package-v0.55.1) (2024-09-17) ### Features * CI deploy on sepolia ([#8514](#8514)) ([54f0344](54f0344)) * Tx verification & peer scoring on p2p layer. bot support for EasyPrivateToken ([#8298](#8298)) ([beb651f](beb651f)) ### Miscellaneous * Remove ARCHIVER_L1_START_BLOCK ([#8554](#8554)) ([bc8d461](bc8d461)) </details> <details><summary>barretenberg.js: 0.55.1</summary> ## [0.55.1](barretenberg.js-v0.55.0...barretenberg.js-v0.55.1) (2024-09-17) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.55.1</summary> ## [0.55.1](aztec-packages-v0.55.0...aztec-packages-v0.55.1) (2024-09-17) ### Features * `TXE::store_note_in_cache` --> `TXE::add_note` ([#8547](#8547)) ([5a6aaeb](5a6aaeb)) * Add a `comptime` string type for string handling at compile-time (noir-lang/noir#6026) ([cd7983a](cd7983a)) * CI deploy on sepolia ([#8514](#8514)) ([54f0344](54f0344)) * Default to outputting witness with file named after package (noir-lang/noir#6031) ([cd7983a](cd7983a)) * Let LSP suggest trait impl methods as you are typing them (noir-lang/noir#6029) ([cd7983a](cd7983a)) * NFT with "transient" storage shield flow ([#8129](#8129)) ([578f67c](578f67c)) * Optimize allocating immediate amounts of memory ([#8579](#8579)) ([e0185e7](e0185e7)) * Spartan iac ([#8455](#8455)) ([16fba46](16fba46)) * Sync from aztec-packages (noir-lang/noir#6028) ([cd7983a](cd7983a)) * Tx verification & peer scoring on p2p layer. bot support for EasyPrivateToken ([#8298](#8298)) ([beb651f](beb651f)) * Unconstraining keys in unconstrained encryption ([#7912](#7912)) ([eb9275a](eb9275a)) * Update args hash to be a flat poseidon ([#8571](#8571)) ([0c54224](0c54224)) * Use poseidon for fn selectors ([#8239](#8239)) ([41891db](41891db)) ### Bug Fixes * Disable side-effects for no_predicates functions (noir-lang/noir#6027) ([cd7983a](cd7983a)) * Native world state test issues ([#8546](#8546)) ([aab8773](aab8773)) * Remove special case for epoch 0 ([#8549](#8549)) ([b035d01](b035d01)) * Serialize AvmVerificationKeyData ([#8529](#8529)) ([78c94a4](78c94a4)) ### Miscellaneous * 7791: Disable world_state test suite ([#8594](#8594)) ([ee21583](ee21583)) * Add jq to aztec image ([#8542](#8542)) ([a7fb791](a7fb791)) * Add sync suite ([#8550](#8550)) ([ce0a9db](ce0a9db)) * **ci:** Action to redo typo PRs ([#8553](#8553)) ([3ed5879](3ed5879)) * **ci:** Fix master ([#8534](#8534)) ([47c368f](47c368f)) * **ci:** Fix redo-typo-pr.yml ([abf9802](abf9802)) * **ci:** Fix redo-typo-pr.yml ([#8555](#8555)) ([7f1673c](7f1673c)) * **ci:** Hotfix ([ffd31aa](ffd31aa)) * **ci:** Hotfix arm ci ([979f267](979f267)) * **ci:** Optimize disk usage in arm run ([#8564](#8564)) ([33e6aa4](33e6aa4)) * **ci:** Use labels and if branch=master to control jobs ([#8508](#8508)) ([68a2226](68a2226)) * GitHub Actions Deployments to Amazon EKS ([#8563](#8563)) ([6fae8f0](6fae8f0)) * Moves add gate out of aux ([#8541](#8541)) ([c3ad163](c3ad163)) * Protogalaxy verifier matches prover 2 ([#8477](#8477)) ([58882b1](58882b1)) * Redo typo PR by ankushgoel27 ([#8595](#8595)) ([7ca6d24](7ca6d24)) * Redo typo PR by Ocheretovich ([#8559](#8559)) ([c4296ba](c4296ba)) * Redo typo PR by Olexandr88 ([#8560](#8560)) ([e35d148](e35d148)) * Redo typo PR by skaunov ([#8557](#8557)) ([8a1e7c3](8a1e7c3)) * Release Noir(0.34.0) (noir-lang/noir#5692) ([cd7983a](cd7983a)) * Remove ARCHIVER_L1_START_BLOCK ([#8554](#8554)) ([bc8d461](bc8d461)) * Remove redundant e2e tests and organize ([#8561](#8561)) ([de2b775](de2b775)) * Remove unused imports ([#8556](#8556)) ([e11242e](e11242e)) * Replace relative paths to noir-protocol-circuits ([2336986](2336986)) * Replace relative paths to noir-protocol-circuits ([9668ed5](9668ed5)) </details> <details><summary>barretenberg: 0.55.1</summary> ## [0.55.1](barretenberg-v0.55.0...barretenberg-v0.55.1) (2024-09-17) ### Bug Fixes * Native world state test issues ([#8546](#8546)) ([aab8773](aab8773)) ### Miscellaneous * 7791: Disable world_state test suite ([#8594](#8594)) ([ee21583](ee21583)) * Moves add gate out of aux ([#8541](#8541)) ([c3ad163](c3ad163)) * Protogalaxy verifier matches prover 2 ([#8477](#8477)) ([58882b1](58882b1)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.55.1</summary> ## [0.55.1](AztecProtocol/aztec-packages@aztec-package-v0.55.0...aztec-package-v0.55.1) (2024-09-17) ### Features * CI deploy on sepolia ([#8514](AztecProtocol/aztec-packages#8514)) ([54f0344](AztecProtocol/aztec-packages@54f0344)) * Tx verification & peer scoring on p2p layer. bot support for EasyPrivateToken ([#8298](AztecProtocol/aztec-packages#8298)) ([beb651f](AztecProtocol/aztec-packages@beb651f)) ### Miscellaneous * Remove ARCHIVER_L1_START_BLOCK ([#8554](AztecProtocol/aztec-packages#8554)) ([bc8d461](AztecProtocol/aztec-packages@bc8d461)) </details> <details><summary>barretenberg.js: 0.55.1</summary> ## [0.55.1](AztecProtocol/aztec-packages@barretenberg.js-v0.55.0...barretenberg.js-v0.55.1) (2024-09-17) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-packages: 0.55.1</summary> ## [0.55.1](AztecProtocol/aztec-packages@aztec-packages-v0.55.0...aztec-packages-v0.55.1) (2024-09-17) ### Features * `TXE::store_note_in_cache` --> `TXE::add_note` ([#8547](AztecProtocol/aztec-packages#8547)) ([5a6aaeb](AztecProtocol/aztec-packages@5a6aaeb)) * Add a `comptime` string type for string handling at compile-time (noir-lang/noir#6026) ([cd7983a](AztecProtocol/aztec-packages@cd7983a)) * CI deploy on sepolia ([#8514](AztecProtocol/aztec-packages#8514)) ([54f0344](AztecProtocol/aztec-packages@54f0344)) * Default to outputting witness with file named after package (noir-lang/noir#6031) ([cd7983a](AztecProtocol/aztec-packages@cd7983a)) * Let LSP suggest trait impl methods as you are typing them (noir-lang/noir#6029) ([cd7983a](AztecProtocol/aztec-packages@cd7983a)) * NFT with "transient" storage shield flow ([#8129](AztecProtocol/aztec-packages#8129)) ([578f67c](AztecProtocol/aztec-packages@578f67c)) * Optimize allocating immediate amounts of memory ([#8579](AztecProtocol/aztec-packages#8579)) ([e0185e7](AztecProtocol/aztec-packages@e0185e7)) * Spartan iac ([#8455](AztecProtocol/aztec-packages#8455)) ([16fba46](AztecProtocol/aztec-packages@16fba46)) * Sync from aztec-packages (noir-lang/noir#6028) ([cd7983a](AztecProtocol/aztec-packages@cd7983a)) * Tx verification & peer scoring on p2p layer. bot support for EasyPrivateToken ([#8298](AztecProtocol/aztec-packages#8298)) ([beb651f](AztecProtocol/aztec-packages@beb651f)) * Unconstraining keys in unconstrained encryption ([#7912](AztecProtocol/aztec-packages#7912)) ([eb9275a](AztecProtocol/aztec-packages@eb9275a)) * Update args hash to be a flat poseidon ([#8571](AztecProtocol/aztec-packages#8571)) ([0c54224](AztecProtocol/aztec-packages@0c54224)) * Use poseidon for fn selectors ([#8239](AztecProtocol/aztec-packages#8239)) ([41891db](AztecProtocol/aztec-packages@41891db)) ### Bug Fixes * Disable side-effects for no_predicates functions (noir-lang/noir#6027) ([cd7983a](AztecProtocol/aztec-packages@cd7983a)) * Native world state test issues ([#8546](AztecProtocol/aztec-packages#8546)) ([aab8773](AztecProtocol/aztec-packages@aab8773)) * Remove special case for epoch 0 ([#8549](AztecProtocol/aztec-packages#8549)) ([b035d01](AztecProtocol/aztec-packages@b035d01)) * Serialize AvmVerificationKeyData ([#8529](AztecProtocol/aztec-packages#8529)) ([78c94a4](AztecProtocol/aztec-packages@78c94a4)) ### Miscellaneous * 7791: Disable world_state test suite ([#8594](AztecProtocol/aztec-packages#8594)) ([ee21583](AztecProtocol/aztec-packages@ee21583)) * Add jq to aztec image ([#8542](AztecProtocol/aztec-packages#8542)) ([a7fb791](AztecProtocol/aztec-packages@a7fb791)) * Add sync suite ([#8550](AztecProtocol/aztec-packages#8550)) ([ce0a9db](AztecProtocol/aztec-packages@ce0a9db)) * **ci:** Action to redo typo PRs ([#8553](AztecProtocol/aztec-packages#8553)) ([3ed5879](AztecProtocol/aztec-packages@3ed5879)) * **ci:** Fix master ([#8534](AztecProtocol/aztec-packages#8534)) ([47c368f](AztecProtocol/aztec-packages@47c368f)) * **ci:** Fix redo-typo-pr.yml ([abf9802](AztecProtocol/aztec-packages@abf9802)) * **ci:** Fix redo-typo-pr.yml ([#8555](AztecProtocol/aztec-packages#8555)) ([7f1673c](AztecProtocol/aztec-packages@7f1673c)) * **ci:** Hotfix ([ffd31aa](AztecProtocol/aztec-packages@ffd31aa)) * **ci:** Hotfix arm ci ([979f267](AztecProtocol/aztec-packages@979f267)) * **ci:** Optimize disk usage in arm run ([#8564](AztecProtocol/aztec-packages#8564)) ([33e6aa4](AztecProtocol/aztec-packages@33e6aa4)) * **ci:** Use labels and if branch=master to control jobs ([#8508](AztecProtocol/aztec-packages#8508)) ([68a2226](AztecProtocol/aztec-packages@68a2226)) * GitHub Actions Deployments to Amazon EKS ([#8563](AztecProtocol/aztec-packages#8563)) ([6fae8f0](AztecProtocol/aztec-packages@6fae8f0)) * Moves add gate out of aux ([#8541](AztecProtocol/aztec-packages#8541)) ([c3ad163](AztecProtocol/aztec-packages@c3ad163)) * Protogalaxy verifier matches prover 2 ([#8477](AztecProtocol/aztec-packages#8477)) ([58882b1](AztecProtocol/aztec-packages@58882b1)) * Redo typo PR by ankushgoel27 ([#8595](AztecProtocol/aztec-packages#8595)) ([7ca6d24](AztecProtocol/aztec-packages@7ca6d24)) * Redo typo PR by Ocheretovich ([#8559](AztecProtocol/aztec-packages#8559)) ([c4296ba](AztecProtocol/aztec-packages@c4296ba)) * Redo typo PR by Olexandr88 ([#8560](AztecProtocol/aztec-packages#8560)) ([e35d148](AztecProtocol/aztec-packages@e35d148)) * Redo typo PR by skaunov ([#8557](AztecProtocol/aztec-packages#8557)) ([8a1e7c3](AztecProtocol/aztec-packages@8a1e7c3)) * Release Noir(0.34.0) (noir-lang/noir#5692) ([cd7983a](AztecProtocol/aztec-packages@cd7983a)) * Remove ARCHIVER_L1_START_BLOCK ([#8554](AztecProtocol/aztec-packages#8554)) ([bc8d461](AztecProtocol/aztec-packages@bc8d461)) * Remove redundant e2e tests and organize ([#8561](AztecProtocol/aztec-packages#8561)) ([de2b775](AztecProtocol/aztec-packages@de2b775)) * Remove unused imports ([#8556](AztecProtocol/aztec-packages#8556)) ([e11242e](AztecProtocol/aztec-packages@e11242e)) * Replace relative paths to noir-protocol-circuits ([2336986](AztecProtocol/aztec-packages@2336986)) * Replace relative paths to noir-protocol-circuits ([9668ed5](AztecProtocol/aztec-packages@9668ed5)) </details> <details><summary>barretenberg: 0.55.1</summary> ## [0.55.1](AztecProtocol/aztec-packages@barretenberg-v0.55.0...barretenberg-v0.55.1) (2024-09-17) ### Bug Fixes * Native world state test issues ([#8546](AztecProtocol/aztec-packages#8546)) ([aab8773](AztecProtocol/aztec-packages@aab8773)) ### Miscellaneous * 7791: Disable world_state test suite ([#8594](AztecProtocol/aztec-packages#8594)) ([ee21583](AztecProtocol/aztec-packages@ee21583)) * Moves add gate out of aux ([#8541](AztecProtocol/aztec-packages#8541)) ([c3ad163](AztecProtocol/aztec-packages@c3ad163)) * Protogalaxy verifier matches prover 2 ([#8477](AztecProtocol/aztec-packages#8477)) ([58882b1](AztecProtocol/aztec-packages@58882b1)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Adding standard tx validation that we perform on Aztec Node, in the P2P layer as well, when receiving a TX from peer.
This is ground work for peer scoring, where depending on the validation that fails, it will also affect gossipsub peer scores