Skip to content

fix: validate private double spends in txs with public funcs#13088

Merged
spalladino merged 4 commits intoAztecProtocol:masterfrom
natebeauregard:nb/fix-double-spend
Mar 28, 2025
Merged

fix: validate private double spends in txs with public funcs#13088
spalladino merged 4 commits intoAztecProtocol:masterfrom
natebeauregard:nb/fix-double-spend

Conversation

@natebeauregard
Copy link
Contributor

@natebeauregard natebeauregard commented Mar 27, 2025 •
edited
Loading

Fixes #12902

The post-processing double-spend validation broke once the PublicTxSimulator+AVM started performing all nullifier insertions.

This PR removes the post-processing double spend validation and instead checks all txs (regardless of if the tx is private-only or has public calls) for duplicate nullifiers during pre-processing. Then, the PublicTxSimulator+AVM will ensure that any nullifiers added during public execution cannot cause collisions.

see https://demerzelsolutions.slack.com/archives/C04BTJAA694/p1743001880033519 for additional info

dbanks12
dbanks12 reviewed Mar 27, 2025
View reviewed changes
Copy link
Contributor

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting that the only post-processing validation we were doing was checking double spends. @spalladino do you think it's alright to completely remove post-processing validation like this?

@dbanks12 dbanks12 added the ci-external-once Run CI on an external PR, but only once. label Mar 27, 2025
@dbanks12 dbanks12 requested review from spalladino and removed request for fcarreiro and sirasistant March 27, 2025 16:14
@dbanks12 dbanks12 added ci-external Allow CI to run on this external pull request. and removed ci-external-once Run CI on an external PR, but only once. ci-external Allow CI to run on this external pull request. labels Mar 27, 2025
spalladino
spalladino approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's add a test that catches this somehow. I'm fine if we have to go all the way up to an e2e test, but we should assert that:

  • If we send a tx that emits a duplicate nullifier in public app logic, it is mined and reverts
  • If we send a tx that emits a duplicate nullifier in public setup logic, it is not included

It's possible there are already e2e tests for that, but let's double check. Doesn't need to be on this PR btw.

@natebeauregard
Copy link
Contributor Author

natebeauregard commented Mar 27, 2025

Looks good, but let's add a test that catches this somehow. I'm fine if we have to go all the way up to an e2e test, but we should assert that:

  • If we send a tx that emits a duplicate nullifier in public app logic, it is mined and reverts
  • If we send a tx that emits a duplicate nullifier in public setup logic, it is not included

It's possible there are already e2e tests for that, but let's double check. Doesn't need to be on this PR btw.

@spalladino Good idea, I think that this is probably important enough to include in this PR before merging to ensure that the full flow is working as expected.

If we send a tx that emits a duplicate nullifier in public app logic, it is mined and reverts

I noticed that there was an e2e test (e2e_private_voting_contract) that should already handle this case. However, my changes above cause the assertion to see the tx reject throw with APP_LOGIC_REVERTED tx status fail and instead just throws the basic "Transaction 0x0b5eac8b283d70c750f0b777df3f0b921daba41694926145b861ad8ba597073b was dropped. Reason: Tx dropped by P2P node" message. I'm currently debugging this to figure out what's going on.

If we send a tx that emits a duplicate nullifier in public setup logic, it is not included

I can add a new e2e test case to assert this behavior.

@natebeauregard
Copy link
Contributor Author

natebeauregard commented Mar 28, 2025 •
edited
Loading

I added commits to update the existing unit/e2e tests and add in a new e2e test for txs that emit a duplicate nullifier in public app logic. After talking with @spalladino , he suggested that we skip adding the e2e test for public setup logic for now and split that out into a separate issue.

I didn't add the new e2e test to CI yet since external contributors can't merge in CI changes, however I verified that the test passed locally. @spalladino @dbanks12 Would either of you be willing to make a small PR once this goes in to add the new test to CI? Thanks in advance.

@spalladino spalladino merged commit 4555871 into AztecProtocol:master Mar 28, 2025
7 checks passed
@AztecBot AztecBot mentioned this pull request Mar 28, 2025
Merged
spalladino added a commit that referenced this pull request Mar 28, 2025
@spalladino
chore: Add e2e test to bootstrap test_cmds
9cb0a46 
Adds test added in #13088
@spalladino spalladino mentioned this pull request Mar 28, 2025
Merged
@natebeauregard natebeauregard deleted the nb/fix-double-spend branch March 28, 2025 18:30
spalladino added a commit that referenced this pull request Mar 28, 2025
@spalladino
chore: Add e2e test to bootstrap test_cmds (#13146)
09e4722 
Adds test added in #13088
@natebeauregard natebeauregard mentioned this pull request Mar 28, 2025
Open
charlielye pushed a commit that referenced this pull request Apr 3, 2025
@AztecBot
chore(master): release 0.83.0 (#13112)
c99fb41 
🤖 I have created a new Aztec Packages release
---


##
[0.83.0](v0.82.3...v0.83.0)
(2025-04-03)


### ⚠ BREAKING CHANGES

* operation mouthwash
([#13171](#13171))
* change version and protocol version to rollupVersion
([#13145](#13145))
* processing events in Aztec.nr
([#12957](#12957))

### Features

* accept multiple consensus client endpoints
([#13022](#13022))
([1d0185d](1d0185d))
* add ecdsa non ssh account to cli wallet
([#13085](#13085))
([c5f9984](c5f9984))
* Add parent log link to top of CI logs.
([#13170](#13170))
([a851087](a851087))
* Add public data read gadget
([#13138](#13138))
([6bb76db](6bb76db))
* **avm:** merkle db hints (part 3)
([#13199](#13199))
([7f96676](7f96676))
* **Barretenberg:** static analyzer's routine
([#13207](#13207))
([84890c2](84890c2))
* derived pending notes capsules slot
([#13102](#13102))
([6307ba0](6307ba0))
* get mana limit from rollup by default.
([#13029](#13029))
([a406c54](a406c54))
* Increase CIVC depth with no rollup cost
([#13106](#13106))
([3a555ef](3a555ef))
* making SyncDataProvider throw before sync
([#13151](#13151))
([9840241](9840241))
* more benchmarks
([#13103](#13103))
([7a2c9b7](7a2c9b7))
* **noir:** Allow missing optional fields in msgpack
([#13141](#13141))
([493dede](493dede))
* processing events in Aztec.nr
([#12957](#12957))
([88c0e04](88c0e04))
* Prover id defaults to publisher address
([#13206](#13206))
([f459c3e](f459c3e))
* purge of log decryption in TS
([#12992](#12992))
([800ab8d](800ab8d))
* register private-only contracts in cli-wallet and misc improvements
([#13245](#13245))
([ad0530f](ad0530f))
* remove unary trick in decomposition and constraints polishing
([#13080](#13080))
([0e60255](0e60255))
* split public inputs from proof
([#12816](#12816))
([ca7151e](ca7151e))
* use magic address for fee juice portal in msgs
([#13241](#13241))
([6fbc378](6fbc378))
* util for computing proposer/forwarder address
([#13169](#13169))
([87809b2](87809b2))
* Zw/goblin avm tests
([#12904](#12904))
([baea4b3](baea4b3))


### Bug Fixes

* add check for rollup version in tx validator
([#13197](#13197))
([c8220c9](c8220c9)),
closes
[#13192](#13192)
* add flake
([13bdfa7](13bdfa7))
* add flakes
([547d50a](547d50a))
* **avm:** alu interface
([#13115](#13115))
([101ff78](101ff78))
* await transaction to make proposal
([#13132](#13132))
([180db9e](180db9e))
* bb mac publish on tag push
([#13174](#13174))
([8d46c8c](8d46c8c))
* bb workdir permission issue
([#13095](#13095))
([3685a80](3685a80))
* **bb:** dont publish .zst
([#13173](#13173))
([4dba4e9](4dba4e9))
* boolean config helper for cli args works now
([#13110](#13110))
([a93ce6e](a93ce6e))
* **docs:** Register FPC docs, contract deployment, events
([#13222](#13222))
([49aabfb](49aabfb))
* don't log sepolia acc mnemonic on github action
([#13178](#13178))
([809b44d](809b44d))
* eq instead of !==
([#13161](#13161))
([921e347](921e347))
* fetch the correct vk in getSolidityVerifier
([#13157](#13157))
([71b6719](71b6719))
* force anvil/blob networking to ipv4 on localhost. attempt to fix port
flakes
([#13099](#13099))
([970dae5](970dae5))
* fuzzing build issues
([#13114](#13114))
([bacae3d](bacae3d))
* Handle proven chain events referring to unseen blocks
([#13144](#13144))
([229515f](229515f)),
closes
[#13142](#13142)
* handling multiple identical logs in a tx
([#13184](#13184))
([c100499](c100499))
* incorrect blocknumber in syncTaggedLogs
([e58ea95](e58ea95))
* indexeddb multimap dupes
([#13254](#13254))
([1e470f0](1e470f0))
* load two more points
([#13119](#13119))
([2a2904a](2a2904a))
* Masternet to run with the nightly tag
([#13239](#13239))
([f16b70e](f16b70e))
* newline weirdness in package.json
([#13111](#13111))
([244ea99](244ea99))
* nightly deploy quotes
([#13253](#13253))
([775eecf](775eecf))
* prover config read from L1
([#13237](#13237))
([13426fe](13426fe))
* Race condition while unwinding blocks
([#13148](#13148))
([1c2291a](1c2291a))
* read and pass rollup version
([#13232](#13232))
([4ee1e4a](4ee1e4a))
* recursive sumcheck bugs
([#12885](#12885))
([a784802](a784802))
* reenable test cache
([4dfca94](4dfca94))
* remove [skip ci] edge condition
([#13228](#13228))
([4e699ea](4e699ea))
* separator in pending partial notes capsule array slot
([#13153](#13153))
([0d6ec63](0d6ec63))
* sysdig oops
([5cf6ad1](5cf6ad1))
* tagging bug
([#13061](#13061))
([280e1a6](280e1a6))
* test tracking failures in merge queue
([#13235](#13235))
([650fdc1](650fdc1))
* title and external check need to run in merge queue
([e326ed6](e326ed6))
* Transpile cmov
([#13194](#13194))
([6b94555](6b94555))
* trying to fix EADDRINUSE
([#13176](#13176))
([260a057](260a057))
* Uninstall gossipsub event handler on service stop
([#13190](#13190))
([081f30d](081f30d))
* use version from registry for rollup instead of config
([#12938](#12938))
([7dac390](7dac390))
* validate private double spends in txs with public funcs
([#13088](#13088))
([4555871](4555871))
* yolo flake and alert fix
([f8de52b](f8de52b))


### Miscellaneous

* add container id to netlog
([9ecdf15](9ecdf15))
* Add e2e test to bootstrap test_cmds
([#13146](#13146))
([09e4722](09e4722))
* add rollup version as universal cli option
([#13205](#13205))
([#13213](#13213))
([568d9e9](568d9e9))
* Add ultra versions of fuzzers in stdlib
([#13139](#13139))
([aea210b](aea210b))
* align TS and C++ indexed leaf types
([#13185](#13185))
([d2574cc](d2574cc))
* Assign bb test flake
([#13127](#13127))
([69fdb04](69fdb04))
* **avm:** remove check_interaction from tests
([#13136](#13136))
([7d875a6](7d875a6))
* change version and protocol version to rollupVersion
([#13145](#13145))
([24d7f8b](24d7f8b))
* Cleanup config vars for alpha-testnet
([#13204](#13204))
([153a606](153a606))
* convenient way to run app ivc from bb
([#13158](#13158))
([cb1a857](cb1a857))
* Cron snapshot upload in spartan
([#13108](#13108))
([7c520a8](7c520a8))
* Do not close store before stopping p2p client in tests
([#13223](#13223))
([63a843e](63a843e))
* docker flake diagnostics.
([48da272](48da272))
* **docs:** alpha-testnet versioning
([#13016](#13016))
([9ca5d3c](9ca5d3c))
* **docs:** Update CLI faucet command
([#13104](#13104))
([6eb71de](6eb71de))
* **docs:** Update versions-updating.md
([#13090](#13090))
([0310d4e](0310d4e))
* Enable debug logging for annoying unit tests
([#13191](#13191))
([5556524](5556524))
* enable sentinel in sentinel test
([#13219](#13219))
([f2cfe3f](f2cfe3f))
* Exclude nightly versions from stable fn
([#13196](#13196))
([a832e90](a832e90))
* Fix flake in e2e fees failures
([#13229](#13229))
([5defe47](5defe47))
* fix kind logs
([#13023](#13023))
([c6c2727](c6c2727)),
closes
[#13053](#13053)
* fuzzing build in ci
([#13105](#13105))
([1c08d38](1c08d38))
* Improve callstacks for public dispatch fns
([#13120](#13120))
([f67375d](f67375d))
* log out the slash factory when a new rollup is deployed
([#13131](#13131))
([4643a31](4643a31))
* Merge alpha back to master
([#13128](#13128))
([504c338](504c338))
* metric attributes
([#13126](#13126))
([f87d5e3](f87d5e3)),
closes
[#13063](#13063)
* minor tagging API improvement
([#13092](#13092))
([f5bcece](f5bcece))
* more benchmarking
([#13211](#13211))
([4b0a4ad](4b0a4ad))
* move unbound impl generics to their functions
([#13147](#13147))
([62f497f](62f497f))
* operation mouthwash
([#13171](#13171))
([384f4e5](384f4e5))
* remove catch-all branch from opcode match in transpiler
([#13210](#13210))
([2bc9ca2](2bc9ca2))
* remove templating by flavor in merge protocol
([#13098](#13098))
([cf5e217](cf5e217))
* rename journal dir and file to state manager & mv to up to public/
([#13159](#13159))
([f13be09](f13be09))
* replace relative paths to noir-protocol-circuits
([f18336d](f18336d))
* replace relative paths to noir-protocol-circuits
([d620c12](d620c12))
* replace relative paths to noir-protocol-circuits
([be3e74e](be3e74e))
* replace relative paths to noir-protocol-circuits
([5bfac63](5bfac63))
* replace relative paths to noir-protocol-circuits
([f41ab2c](f41ab2c))
* replace relative paths to noir-protocol-circuits
([806e560](806e560))
* streamlined log processing
([#13107](#13107))
([b184865](b184865))
* use testnet optimized trace
([#13135](#13135))
([5a4f2ac](5a4f2ac))
* uses a new larger redis cache. flake file tweaks.
([#13167](#13167))
([0951fb6](0951fb6))


### Documentation

* Add intermediate migration note versions
([#13183](#13183))
([f5de7b8](f5de7b8))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbanks12 dbanks12 dbanks12 left review comments

@spalladino spalladino spalladino approved these changes

Assignees

No one assigned

Labels

ci-external Allow CI to run on this external pull request.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: Private double spends in txs with public functions are not validated

3 participants

@natebeauregard @spalladino @dbanks12