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

poet: allow one submission of PoST per miner per round per poet service #3458

Closed
3 tasks done
Tracked by #174
countvonzero opened this issue Aug 18, 2022 · 8 comments
Closed
3 tasks done
Tracked by #174
Assignees
Labels

Comments

@countvonzero
Copy link
Contributor

countvonzero commented Aug 18, 2022

Description

condensing Noam's comments in slack:

To enforce this, the node will need to submit the entire “poet challenge” (NIPostChallenge) to the PoET and it will calculate the hash itself (could be good to return the hash in the response)

This part is not trivial, I think we may want the PoET to ask a node (it already has access to the “PoET gateway” - maybe we should rename it) to validate this data structure.

If this is a smesher’s first ATX, then the NIPostChallenge includes the initial PoST proof - which needs to be validated.

If it’s a subsequent ATX, the NIPostChallenge only includes a reference to the PrevATXID - that previous ATX includes the PoST proof that came after the previous PoET run (we only do one PoST proof each epoch). So validation here must be contextual - the challenge only tells the PoET which smesher it is, but to validate that they still expend resources, we must actually inspect the mesh. This is trivial for the node to do, but not for the context-less PoET.

Tasks:

@moshababo
Copy link
Contributor

moshababo commented Aug 23, 2022

Design proposal

Upon registration, the node should provide:

  • PositioningATX
  • PubLayerID
  • InitialPost + its metadata (for an initial ATX registration)
  • PrevATXID (for a non-initial ATX registration)
  • Signature for a msg containing all other fields

PoET service steps:

  • Verify signature and extract pubkey (as NodeID). The reason for this origin check is that although the utility of the PoET results is bound to the specified NodeID, a non-honest user can register for a given round on behalf of some other node, while providing request data that will eventually yield a non-valid ATX, and so practically preventing the node from using the PoET service for a given round.
  • Verify NodeID to be unregistered for the upcoming round.
  • For an initial ATX
    • Verify the validity of InitialPost with respect to the metadata and NodeID. 1
    • Verify the metadata with respect to consensus rules. 2
  • For a non-initial ATX
    • Fetch PrevATXID ATX. 3
    • Verify NodeID to match the prev ATX's NodeID.
    • Calculate Sequence according to the prev ATX's Sequence.
  • Calculate the challenge hash and apply it for the upcoming round
    • For an initial ATX: sha256( NodeID ++ PubLayerID ++ PositioningATX ++ InitialPostIndices ) 4
    • For a non-initial ATX: sha256( NodeID ++ Sequence ++ PrevATXID ++ PubLayerID ++ PositioningATX )
  • Return the hash in the response, allowing the node to verify its correctness.

Optional

  • Fetch PositioningATX ATX and verify that its PubLayerID is 1 epoch distant from PubLayerID.
  • Verify PubLayerID to be 1 epoch or more distant from the prev ATX’s PubLayerID

If any is invalid, then the produced ATX would be invalid. Allowing this to happen is applicable, but verifying it might allow dropping the requirement for nodes to sign the registration request; it is enforcing validity on the registration data, and by doing so making a registration counterfeit a non-issue: if/when the node will register for the given round, and fail due to the previous registration, it will still be able to listen to the broadcasted results and utilize it.

Footnotes

  1. Using github.com/spacemeshos/post/verifying package as a lib.

  2. Node’s SmesherService.PostConfig RPC can be used (although K1 & K2 fields will need to be added).

  3. Node's MeshService.AccountMeshDataQuery RPC can be used (although it might need to be further optimized to support that properly).

  4. InitialPostIndices is taken from InitialPost.

@noamnelke
Copy link
Member

LGTM

Good job on the structure of this spec! Very readable, keeping the technicalities to the footnotes...

@countvonzero countvonzero moved this to 📋 Backlog in Dev team kanban Sep 26, 2022
@poszu poszu self-assigned this Oct 18, 2022
@poszu poszu moved this from 📋 Backlog to 🔖 Next in Dev team kanban Oct 18, 2022
@poszu poszu moved this from 🔖 Next to 🏗 Doing in Dev team kanban Oct 19, 2022
bors bot pushed a commit that referenced this issue Oct 19, 2022
## Motivation
Structures for Poet's GRPC API are duplicated in go-spacemesh code without good reason.

Prework for: #3458 

## Changes
Removed duplicated structures and use the structures autogenerated from poet's API protosbufs.

## Test Plan
unit tests

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@poszu
Copy link
Contributor

poszu commented Oct 24, 2022

@moshababo What about CommitmentATX? It's part of the NIPostChallenge at the moment:

type NIPostChallenge struct {
// Sequence number counts the number of ancestors of the ATX. It sequentially increases for each ATX in the chain.
// Two ATXs with the same sequence number from the same miner can be used as the proof of malfeasance against that miner.
Sequence uint64
PrevATXID ATXID
PubLayerID LayerID
PositioningATX ATXID
// CommitmentATX is the ATX used in the commitment for initializing the PoST of the node.
CommitmentATX *ATXID
InitialPostIndices []byte
}

@moshababo
Copy link
Contributor

It needs to be added as well (I didn't mention it because it didn't exist at the time I wrote the proposal).

@poszu
Copy link
Contributor

poszu commented Oct 25, 2022

@moshababo, I have a few questions:

It needs to be added as well (I didn't mention it because it didn't exist at the time I wrote the proposal).

Could You please update the design proposal to include it? I guess that the challenge hash for an initial ATX:

sha256( NodeID ++ PubLayerID ++ PositioningATX ++ InitialPostIndices )

should include CommitmentATX too?


Verify the metadata with respect to consensus rules.

Could You please point out these rules?


Node’s SmesherService.PostConfig RPC can be used

Node's MeshService.AccountMeshDataQuery RPC can be used

Do I understand correctly that Poet is supposed to call back the Node that submitted challenge data to ask it for more details?

These details will always be required, so perhaps they should be passed in the initial Submit() GRPC? For example, instead of sending just PreviousATXId, send the whole ATX.

Alternatively, to avoid figuring out how to call back the node and create a grpc client for it etc., a bidirectional GRPC stream could be used. The server (Poet) would send commands in response to Submit() call to fetch additional data.
It's been proposed here: https://stackoverflow.com/a/53257570/2806736

These commands could be in form of oneof like:

rpc Submit (stream SubmitRequest) returns (stream SubmitResponse) {}

message ATX {
    ....
}

message SubmitRequest {
    message ChallengeData { 
        ...
    }
    oneof cmd {
        ChallengeData submit = 1;
        ATX atx = 2;
    }
}

message SubmitResponse {
    message Finished {
        string roundId = 1;
        bytes challenge_hash = 2;
    }
    oneof cmd {
        bytes fetch_adx = 1;
        Finished finished = 2;
    }
}

@poszu
Copy link
Contributor

poszu commented Oct 26, 2022

@moshababo few more questions:

Calculate Sequence according to the prev ATX's Sequence.

Why not just pass the sequence in the challenge data?


Calculate the challenge hash and apply it for the upcoming round

  • For an initial ATX: sha256( NodeID ++ PubLayerID ++ PositioningATX ++ InitialPostIndices )
  • For a non-initial ATX: sha256( NodeID ++ Sequence ++ PrevATXID ++ PubLayerID ++ PositioningATX )

Challenge data must be serialized consistently anyway for signing. How about we use the serialized representation for hashing too? Then it would become consistent for both cases (initial and non-initial):

sha256(NodeID ++ codec.Encode(challengeData))

(I cannot decide ATM if NodeID should be part of challengeData).


What about the structure of an ATX? It currently contains the NIPostChallenge structure defined in go-spacemesh/types/activation.go. I feel we should avoid defining the same data in multiple places. Perhaps it should contain the same data that were submitted to Poet?

@moshababo
Copy link
Contributor

It needs to be added as well (I didn't mention it because it didn't exist at the time I wrote the proposal).

Could You please update the design proposal to include it? I guess that the challenge hash for an initial ATX:

sha256( NodeID ++ PubLayerID ++ PositioningATX ++ InitialPostIndices )

should include CommitmentATX too?

Yes, the challenge hash should include it.

I think that updating the original proposal comment isn’t the best approach (assuming it won’t be the last time). I suggest that you’ll maintain a snapshot of the latest design/impl specs elsewhere, or just consider the entire thread for that purpose.


Verify the metadata with respect to consensus rules.

Could You please point out these rules?

The PoST consensus rules are currently defined in the node’s config, with the following params: BitsPerLabel, LabelsPerUnit, MaxNumUnits, MinNumUnits, K1, K2. The given NumUnits in the metadata needs to be checked for range, while the rest should just be equal.


Node’s SmesherService.PostConfig RPC can be used
Node's MeshService.AccountMeshDataQuery RPC can be used

Do I understand correctly that Poet is supposed to call back the Node that submitted challenge data to ask it for more details?
These details will always be required, so perhaps they should be passed in the initial Submit() GRPC? For example, instead of sending just PreviousATXId, send the whole ATX.

The PoST config are network-level params, so they will need to be queried just once.
The prev-ATX query will happen frequently, but getting it from the submitting client is problematic because it cannot be considered as an honest party (hence why this feature is implemented in the first place). The PoET server’s gateway node(s) are the ones to be trusted (and can be considered as part of the same operation, running the node’s implementation as a service).


Calculate Sequence according to the prev ATX's Sequence.
Why not just pass the sequence in the challenge data?

The value is ought to be the prev sequence + 1. So I didn’t see much utility in passing and then verifying it, instead of just computing it. But passing it might allow better flexibility for future use cases.


Calculate the challenge hash and apply it for the upcoming round

  • For an initial ATX: sha256( NodeID ++ PubLayerID ++ PositioningATX ++ InitialPostIndices )
  • For a non-initial ATX: sha256( NodeID ++ Sequence ++ PrevATXID ++ PubLayerID ++ PositioningATX )

Challenge data must be serialized consistently anyway for signing. How about we use the serialized representation for hashing too? > Then it would become consistent for both cases (initial and non-initial):

sha256(NodeID ++ codec.Encode(challengeData))

(I cannot decide ATM if NodeID should be part of challengeData).

This serialization and hashing are currently done in the node. The implementation purpose is to mimic it. Appending well-defined byte arrays is the simplest way to serialize, and the usual practice for producing preimages. I’m not sure that incorporating a more robust serialization scheme is needed here, even if it’s used elsewhere.

What about the structure of an ATX? It currently contains the NIPostChallenge structure defined in go-spacemesh/types/activation.go. I feel we should avoid defining the same data in multiple places. Perhaps it should contain the same data that were submitted to Poet?

The ATX wire serialization is defined in the node's gRPC API via protobuf. Once fetched, that should be sufficient in order to perform the associated checks. Why else would you need to define it separately?

bors bot pushed a commit that referenced this issue Nov 10, 2022
## Motivation
The extra K1 and K2 are needed in Poet for #3458
Closes #3707 

## Changes
Fill K1 and K2 in `SmesherService::PostConfig`

## Test Plan
unit tests

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot pushed a commit that referenced this issue Nov 29, 2022
## Motivation
Implemented service for verifying poet challenges by its trusted gateway. Poet is supposed to relay submitted challenges to its gateway for verification and hashing.
Part of #3458 

## Test Plan
- unit tests
- tested e2e (systests) with updated poet POC using this service

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot pushed a commit that referenced this issue Nov 29, 2022
## Motivation
Part of #3458 
Closes #3708

## Changes
Updated the poet client and the nipost builder to use the new extended Poet Submit API.

## Test Plan
- unit tests
- e2e system tests with the new Poet

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot pushed a commit that referenced this issue Nov 30, 2022
## Motivation
Part of #3458 
Closes #3708

## Changes
Updated the poet client and the nipost builder to use the new extended Poet Submit API.

## Test Plan
- unit tests
- e2e system tests with the new Poet

## DevOps Notes
- [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [x] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
@poszu
Copy link
Contributor

poszu commented Nov 30, 2022

Done

@poszu poszu closed this as completed Nov 30, 2022
Repository owner moved this from 🏗 Doing to ✅ Done in Dev team kanban Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

4 participants