Skip to content
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

Add transaction tag for fraud proof and re-define fraud proof priority #2617

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

NingLin-P
Copy link
Member

@NingLin-P NingLin-P commented Mar 14, 2024

This PR implement #2570, include changes of:

  • Add transaction tag for fraud proof
    • For fraud proof that targets bad ER, for a given domain at most one fraud proof will be accepted at a time
    • For bundle equivocation fraud proof, for a given operator at most one fraud proof will be accepted at a time
  • Re-define fraud proof priority
    • Fraud proof priority is defined as MAX - block_before_bad_er_confirm, thus fraud proof that targets closer to confirm bad ER (more emergency) has a higher priority to be accepted by tx pool and to be included in block.
  • Reduce the priority of bundle and XDM from MAX to 1
    • So the bundle and XDM have a higher priority than other normal extrinsic (which has 0 priority), but less than the fraud proof

There is a TODO left in this PR, although an incoming fraud proof will be rejected if there is already a fraud proof with higher priority accepted by the tx pool but it is only rejected after it is validated when the cost is already made, we can avoid this cost by checking if a fraud proof with the same tag and higher priority already exists in the pool but unfortunately upstream doesn't support query tx by tag yet, I have open paritytech/polkadot-sdk#3705 for this feature.

I also tried to write a test to submit multiple fraud proofs and see the behavior, but the operator only produced fraud proof for the oldest bad ER and it is too hack to manually construct a fraud proof, but after the above TODO is resolved fraud proof will be rejected before it is validated thus we can use a dummy fraud proof, so will add the test by then.

Code contributor checklist:

The provides tag ensure for a given domain there is only at most one fraud proof
acceptted by the tx pool at a time. The priority ensure more emergency fraud proof
have higher priority to be acceptted by tx pool and  included in the block

Signed-off-by: linning <linningde25@gmail.com>
Bundle and XDM have a bit higher priority than normal extrinsic but must less than
the fraud proof

Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
@dariolina
Copy link
Member

Please add a section like "Fraud proof priority in tx pool" with a few sentences like you have in the description to the bottom of this file https://github.com/subspace/protocol-specs/blob/main/docs/decex/fraud_proofs.md (I may find a better place for it later)

@dariolina
Copy link
Member

The spec was updated and this looks good to me. @vedhavyas could you take a final look at the code?

vedhavyas
vedhavyas previously approved these changes Mar 28, 2024
@NingLin-P NingLin-P added this pull request to the merge queue Mar 28, 2024
Merged via the queue into main with commit 5566f4b Mar 28, 2024
11 checks passed
@NingLin-P NingLin-P deleted the fp-tag-priority branch March 28, 2024 13:41
@vanhauser-thc
Copy link
Collaborator

We also found this issue at the same time.
@dariolina mabe this PR should have the "need audit" flag?

@dariolina dariolina added the need to audit This change needs to be audited label Apr 8, 2024
@nazar-pc nazar-pc mentioned this pull request Apr 15, 2024
1 task
@vanhauser-thc vanhauser-thc added audited This change was audited and removed need to audit This change needs to be audited labels Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audited This change was audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants