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

[Merged by Bors] - Detect double PoST inclusion malfeasance #6117

Closed
wants to merge 9 commits into from

Conversation

poszu
Copy link
Contributor

@poszu poszu commented Jul 10, 2024

Motivation

With ATX merge, a dishonest smesher might try to include his PoST twice for doubled rewards:

  • once in a merged ATX (published by another identity)
  • once in a self-published ATX

Description

Detect if any of the IDs participating in the processed ATX has already contributed its PoST to another ATX in the epoch

💡 This PR doesn't implement the malfeasance proof, just malfeasance detection. The proof is stubbed and will come in a later PR.

Test Plan

added tests

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 87.75510% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.9%. Comparing base (2899a93) to head (3fe49b3).
Report is 3 commits behind head on develop.

Files Patch % Lines
activation/handler_v2.go 85.1% 3 Missing and 1 partial ⚠️
sql/atxs/atxs.go 90.9% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #6117   +/-   ##
=======================================
  Coverage     81.9%   81.9%           
=======================================
  Files          308     308           
  Lines        33795   33843   +48     
=======================================
+ Hits         27705   27750   +45     
- Misses        4318    4319    +1     
- Partials      1772    1774    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sql/atxs/atxs.go Outdated Show resolved Hide resolved
sql/atxs/atxs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ivan4th ivan4th 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, might want convert SQL to use INNER JOIN

@mathcrypto mathcrypto self-requested a review July 26, 2024 13:14
sql/atxs/atxs_test.go Show resolved Hide resolved
db := sql.InMemory()

// one atx
atx0, blob := newAtx(t, sig, withPublishEpoch(1))
Copy link
Member

Choose a reason for hiding this comment

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

newAtx returns a wire.ActivationTxV1 as blob. I think newAtx doesn't need to return any wire type for the tests here, if a blob is needed for a test it should generally be possible to just use types.RandomBytes(20) since the blob should not be needed to be decoded in any test in this package.

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't say I like returning a fake value when a real one is so easy to use. It's a footgun waiting to shoot somebody in the future. However, the blob could be obtained by calling atx0.Blob(), so we can only return the ATX.

I will refactor it to return only ATX in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

The problem I see with returning a real value is that for this we probably will need a dependency on activation and/or activation/wire but sql packages shouldn't depend on those (cyclic dependencies). Using xxx_test packages for tests is a way around this, but since sql packages can't even decode the bytes by themselves I don't see an issue with faking the bytes that are stored / returned in tests.

@poszu
Copy link
Contributor Author

poszu commented Aug 5, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Aug 5, 2024
## Motivation

With ATX merge, a dishonest smesher might try to include his PoST twice for doubled rewards:
- once in a merged ATX (published by another identity)
- once in a self-published ATX
@spacemesh-bors
Copy link

spacemesh-bors bot commented Aug 5, 2024

Build failed:

@poszu
Copy link
Contributor Author

poszu commented Aug 5, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Aug 5, 2024
## Motivation

With ATX merge, a dishonest smesher might try to include his PoST twice for doubled rewards:
- once in a merged ATX (published by another identity)
- once in a self-published ATX
@spacemesh-bors
Copy link

spacemesh-bors bot commented Aug 5, 2024

Build failed (retrying...):

spacemesh-bors bot pushed a commit that referenced this pull request Aug 5, 2024
## Motivation

With ATX merge, a dishonest smesher might try to include his PoST twice for doubled rewards:
- once in a merged ATX (published by another identity)
- once in a self-published ATX
@spacemesh-bors
Copy link

spacemesh-bors bot commented Aug 5, 2024

Build failed:

@poszu
Copy link
Contributor Author

poszu commented Aug 5, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Aug 5, 2024
## Motivation

With ATX merge, a dishonest smesher might try to include his PoST twice for doubled rewards:
- once in a merged ATX (published by another identity)
- once in a self-published ATX
@spacemesh-bors
Copy link

spacemesh-bors bot commented Aug 5, 2024

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title Detect double PoST inclusion malfeasance [Merged by Bors] - Detect double PoST inclusion malfeasance Aug 5, 2024
@spacemesh-bors spacemesh-bors bot closed this Aug 5, 2024
@spacemesh-bors spacemesh-bors bot deleted the atxmerge/detect-double-post branch August 5, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants