Fix in-circuit hashing of complex account update layouts #1364
Conversation
| @@ -1321,6 +1347,7 @@ class AccountUpdate implements Types.AccountUpdate { | |||
| accountUpdate.body.mayUseToken.inheritFromParent.assertFalse(); | |||
| return; | |||
| } | |||
| accountUpdate.children.callsType = { type: 'None' }; | |||
There was a problem hiding this comment.
this fixed the bug. literally one line 😓
| @@ -1096,11 +1096,37 @@ class AccountUpdate implements Types.AccountUpdate { | |||
| return { accountUpdate, calls }; | |||
| } | |||
|
|
|||
| toPrettyLayout() { | |||
There was a problem hiding this comment.
this was useful for debugging, leaving it in here
|
@mitschabaude, since the DEX app is run on CI now, we would pick it up if this regressed / the bug was reintroduced right? Intuitively, account updates seem like the kind of thing that shoud have good unit test coverage (and be easy to test) so I'm inclined to say it should have a unit test. If that's a lot of work then we could dig into why it's hard and see if that should be fixed? |
Yes, we would pick up if this regressed. DEX was already running in CI but in an insecure version that worked around the bug, which is fixed here We do have unit tests for account updates, but what broke here still seems hard to unit-test. I could devise a test that would catch a regression, but it would be quite artificial, setting up an interaction between multiple calls to Ultimately, I think the culprit for this bug is the overall structure of the code which relies heavily on mutating a shared transaction object, including mutating the individual account updates, from random places in the code. The global structure of the code is bad, and I think it needs some design and refactoring work to become better. The broad idea for me would be to make the global list of account updates largely immutable except in a few well-defined places -- such that the effects of the code fixed here become more local, after which unit tests become meaningful. |
Fixes #706
This is the minimal PR that fixes the bug.
Finding the bug was possible thanks to new infrastructure for catching compile/prove discrepancies, which now lives in a follow-up PR: #1363 (still needs clean-up work, and relies on changes to snarky that need broader alignment)
Note: This doesn't add unit tests. Should it?
It doesn't seem obvious that complex account update layouts can be tested in isolation, since it was the global mutable structure of transactions, and unwanted cross-interactions between proofs at multiple layers of the callstack, that broke them. It seems that integration tests are needed to exhibit non-trivial failure cases, and this PR already fixes such an integration test as its main result