penumbra icon indicating copy to clipboard operation
penumbra copied to clipboard

Make block roots unique for transactions containing `Swap`

Open zbuc opened this issue 2 years ago • 3 comments

The swap design uses the block root to strongly identify which block a Swap transaction belongs to. This is an issue because we can only prove that the note was included in a block with the given root, since the height is not bound to the block root.

From docs/@hdevalence:

all that revealing the block-level root on the authentication path proves is that the note was included in a block with that root, not the block with that root, since the block root binds all of the new note commitments produced in that block but does not explicitly bind the block height. To fix this, we can have the chain insert a dummy note whose note commitment is bound to the block height (e.g., by computing the note’s blinding factor as a hash of the height). This prevents a possible attack where an attacker who could control the exact set of note commitments included in two different blocks at heights h1 and h 2, both with swaps, could submit the exact same input amounts in h 1 and h2 (without change), and then claim both outputs at whichever executed with a higher price.

i think we'll want to insert a chain-produced dummy note into any block that contains a swap

zbuc avatar Jul 13 '22 03:07 zbuc

Capturing some context that was discussed aloud: as we talked about in zoom, it should suffice to use mint_note to a random address for this purpose, since mint_note uses a blinding factor derived from the position of the NCT, which includes the block height as a subcomponent. Since only the shielded pool can mint notes, the swap component will need to write a boolean into the JMT during end_block to indicate whether or not the block contained any swaps, and this can be read during the end_block of the shielded pool to mint the dummy note, if necessary.

plaidfinch avatar Jul 18 '22 15:07 plaidfinch

Possibly no longer necessary due to revised design, pausing for now

redshiftzero avatar Jul 21 '22 20:07 redshiftzero

Removed from milestone because it's not important to do this now, only to do it eventually.

the swap component will need to write a boolean into the JMT

Rather than adding another special case, it would be cleaner to use the mechanism we're already planning to implement for IBC transfers: #979

hdevalence avatar Jul 24 '22 18:07 hdevalence

We don't need this any more, we use the position instead now, since it contains the block height.

hdevalence avatar Nov 16 '22 03:11 hdevalence