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] - ATX V2 Syntactical validation with deps - singular ATXs #5949

Closed

Conversation

poszu
Copy link
Contributor

@poszu poszu commented May 16, 2024

Description

Closes #5994

  • syntactically validate ATX V2 with deps (previous, commitment and positioning ATXs and poet proofs)
  • doesn't support marriage or merged ATXs (denoted in the code with TODO comments)

Test Plan

Added units tests

TODO

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

@poszu poszu force-pushed the atx-handler-v2/fetching-deps branch from e5a59af to 5f0aa80 Compare May 16, 2024 15:17
@poszu poszu force-pushed the atx-handler-v2/syntactical-validation-with-deps branch from cdba26d to 0557204 Compare May 16, 2024 15:18
Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 80.41958% with 56 lines in your changes are missing coverage. Please review.

Project coverage is 81.2%. Comparing base (0db7e0a) to head (1ea1d91).

Files Patch % Lines
activation/handler_v2.go 81.3% 27 Missing and 17 partials ⚠️
activation/validation.go 50.0% 8 Missing ⚠️
sql/atxs/atxs.go 81.2% 1 Missing and 2 partials ⚠️
activation/handler.go 0.0% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #5949    +/-   ##
========================================
  Coverage     81.2%   81.2%            
========================================
  Files          293     293            
  Lines        31123   31403   +280     
========================================
+ Hits         25276   25507   +231     
- Misses        4198    4229    +31     
- Partials      1649    1667    +18     

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

@poszu poszu force-pushed the atx-handler-v2/fetching-deps branch from 5f0aa80 to 71540cd Compare May 23, 2024 15:59
@spacemesh-bors spacemesh-bors bot changed the base branch from atx-handler-v2/fetching-deps to develop May 23, 2024 18:45
@poszu poszu force-pushed the atx-handler-v2/syntactical-validation-with-deps branch 2 times, most recently from 5718c00 to a67e37d Compare May 24, 2024 11:14
@poszu poszu force-pushed the atx-handler-v2/syntactical-validation-with-deps branch from 06a2e9e to fe27835 Compare May 24, 2024 12:00
@poszu poszu marked this pull request as ready for review May 24, 2024 13:32
activation/handler_v2.go Outdated Show resolved Hide resolved

func (h *HandlerV2) validateCommitmentAtx(golden, commitmentAtxId types.ATXID, publish types.EpochID) error {
if commitmentAtxId != golden {
commitment, err := atxs.Get(h.cdb, commitmentAtxId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Querying the full set of ATX fields is not very efficient. For one thing, if we don't need the blob of the commitment ATX here, and doing JOIN on the atx_blobs table (which atxs.Get does) will slow down validation process. Another thing to consider is that multiple separate SQL queries are always slower than a single query, even if the single query fetches some excess fields. Maybe we could consider doing a single query for the deps along the lines of

select epoch, nonce, num_units where id in (?, ?, ?)

using IDs of commitment / prev / positioning ATXs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For one thing, if we don't need the blob of the commitment ATX here, and doing JOIN on the atx_blobs table (which atxs.Get does) will slow down validation process.

The atxs.Get() doesn't read the blob anymore. It only reads the atxs table.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure a single query is easily doable during the handling of an incoming ATXs, since there are so many branches that need to be considered depending on

  • initial vs. non-initial
  • merged vs. not-merged
  • ATX that resizes post vs. one that doesn't
  • etc.

We could consider doing all in a single TX and see if that decreased the validation time.


func (h *HandlerV2) previous(ctx context.Context, id types.ATXID) (opaqueAtx, error) {
var blob sql.Blob
version, err := atxs.LoadBlob(ctx, h.cdb, id[:], &blob)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use fields other then those available in the atxs table for previous ATX validation?
That is, do we really need to decode and validate that blob?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this because the ATX in the atxs table contains the effective num units. Effective num units are the smallest value of the num units in the ATX and its previous ATX (the ones declared by the ATX itself). If we take a minimum of Atx.NumUnits and PrevATX.EffectiveNumUnits, then no one will be able to grow effectively their PoST size.

Copy link
Contributor

@ivan4th ivan4th May 28, 2024

Choose a reason for hiding this comment

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

Interesting why then we're storing atx.NumUnits in the table. Do we have some kind of confusion here?

stmt.BindInt64(3, int64(atx.NumUnits))

Copy link
Member

Choose a reason for hiding this comment

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

NumUnits as included by the ATX is only needed to be able to verify the provided post. Effective NumUnits (what we store as NumUnits in the DB) is used to calculate the actual weight of an identity. They are not the same.

To count your NumUnits on disk as effective numunits you need to hold them for at least one full poet round, i.e. you must generate 2 ATXs with that amount of storage - one before the poet round and one after. That's why effective NumUnits is min(previous.NumUnits, atx.NumUnits) (where both are the wire form of the ATX).

activation/handler_v2.go Outdated Show resolved Hide resolved

func (h *HandlerV2) validateCommitmentAtx(golden, commitmentAtxId types.ATXID, publish types.EpochID) error {
if commitmentAtxId != golden {
commitment, err := atxs.Get(h.cdb, commitmentAtxId)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure a single query is easily doable during the handling of an incoming ATXs, since there are so many branches that need to be considered depending on

  • initial vs. non-initial
  • merged vs. not-merged
  • ATX that resizes post vs. one that doesn't
  • etc.

We could consider doing all in a single TX and see if that decreased the validation time.

activation/handler_v2.go Outdated Show resolved Hide resolved
activation/handler_v2.go Outdated Show resolved Hide resolved
@poszu poszu requested review from ivan4th and fasmat May 28, 2024 12:20
@poszu
Copy link
Contributor Author

poszu commented May 29, 2024

bors merge

@spacemesh-bors
Copy link

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title ATX V2 Syntactical validation with deps - singular ATXs [Merged by Bors] - ATX V2 Syntactical validation with deps - singular ATXs May 29, 2024
@spacemesh-bors spacemesh-bors bot closed this May 29, 2024
@spacemesh-bors spacemesh-bors bot deleted the atx-handler-v2/syntactical-validation-with-deps branch May 29, 2024 14:06
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.

syntactical validation of ATX solo V2
3 participants