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] - Smesher API + GPU/Tree-free PoST integration #2149

Closed
wants to merge 108 commits into from

Conversation

moshababo
Copy link
Contributor

@moshababo moshababo commented Sep 24, 2020

Implemented the new Smesher API (instead of the previous Mining API), while integrating GPU/Tree-free PoST.
This PR is based on spacemeshos/post#41, which need to be reviewed first, hence this description won't repeat what was already been specified there.

Main remarks:

  • Created PostSetupManager for implementing the new Post setup API. The previous PostClient was removed, and setup functionalities previously implemented by the ATX builder were removed as well.
  • Refactored the ATX builder to comply with the new Smeshing API. Its dependency with PostSetupManager is only regarding the setup completion status and generation of proofs.
  • In addition to the raw Post, the NIPost now contains the Post metadata as well, that the proof is expected to be verified upon during the syntactic validation. Some of the metadata values are expected to be derived from consensus, hence should be validated in the contextual validation, but it is currently still being derived from the node configuration, so it’s still taking place in the syntactic validation (which will need to remain anyway for validating min/max boundaries).
  • Added PostSetupOpts config. It is used for initiating a new data creation session - either via the Smesher API, or on node start.
  • Renamed the ATX’s commitment as initialPost. As discussed with @tal-m, in the tree-free PoST the proof is not a commitment to the data.
  • Changed NIPoST’S PoET challenge to contain the initial Post indices, instead of its Merkle root, which is no longer applicable. The reason is the same - it is expected to be known at the time the challenge was constructed.
  • Renamed “nipst” as “NIPost” throughout the codebase (closing Rename "Nipst" as "NIPoST" throughout the codebase and docs #1371).
  • Some of the old API code and test-code was commented out or disabled due to compile-time errors caused to the new defined interfaces. It is expected to be fully removed soon.

Copy link
Member

@lrettig lrettig left a comment

Choose a reason for hiding this comment

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

Only reviewed the API changes. See inline comments. Also noticed that you added a few TODO items, it would be nice if they were linked to open issues if they remain when the code is merged.

api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
api/grpcserver/smesher_service.go Outdated Show resolved Hide resolved
@lrettig
Copy link
Member

lrettig commented Sep 25, 2020

Some of this is going to cause conflicts with #2131, which totally removes the old API code. Consider leaving these changes out of this PR, as that one will blow away this code entirely.

@moshababo
Copy link
Contributor Author

@lrettig thanks for reviewing.


Only reviewed the API changes. See inline comments. Also noticed that you added a few TODO items, it would be nice if they were linked to open issues if they remain when the code is merged.

They will be solved prior to merging.

Some of this is going to cause conflicts with #2131, which totally removes the old API code. Consider leaving these changes out of this PR, as that one will blow away this code entirely.

I've made changes only to avoid compile-time and test errors. If you expect #2131 to be merged soon, i'll wait and then solve conflicts.

@lrettig
Copy link
Member

lrettig commented Oct 14, 2020

If you expect #2131 to be merged soon, i'll wait and then solve conflicts.

I'm still working on it, but I expect it to be ready to be merged by next week.

@moshababo moshababo self-assigned this Mar 24, 2021
@avive
Copy link
Contributor

avive commented May 30, 2021

The implementation of the pos progress status stream throws an exception if a pos init process is not currently in progress. The desirable pattern for clients is to subscribe to events on this stream before setting up pos so they can get all errors and status on it while it is ongoing. The stream should be closed by client at any time but never by node (except maybe when running out of resources) . Expecting users to subscribe to stream only after starting a pos init job is not a good pattern because they can miss error events on the stream this way and this is not the normal usage pattern of grpc streaming apis. In addition, I noticed that the stream by the node when a pos setup is complete - this also shouldn't be done and any future pos operation should send data over the stream before the client closed it. I think this is the most straightforward way to implement this kind of functionality.

@moshababo
Copy link
Contributor Author

The implementation of the pos progress status stream throws an exception if a pos init process is not currently in progress. The desirable pattern for clients is to subscribe to events on this stream before setting up pos so they can get all errors and status on it while it is ongoing.

Fixed. (also see spacemeshos/smrepl#23 (comment))

The stream should be closed by client at any time but never by node (except maybe when running out of resources)

In addition, I noticed that the stream [IS CLOSED] by the node when a pos setup is complete - this also shouldn't be done and any future pos operation should send data over the stream before the client closed it. I think this is the most straightforward way to implement this kind of functionality.

I’d rather finalize the stream functionality in a follow-up PR, and focus now on the finishing the general API design and implementation.

@avive
Copy link
Contributor

avive commented Jun 5, 2021

ok

@avive
Copy link
Contributor

avive commented Jun 10, 2021

Building this branch fails on Windows 10 pro with a gcc linker error where building developer branch builds just fine. Likely a GCC issue due to differences between behavior on unix and windows systems. Env: PowerShell / Mingw64.

@moshababo moshababo added this to the 0.2 Release milestone Jun 21, 2021
# Conflicts:
#	.github/workflows/ci.yml
#	DockerFileTests
#	Dockerfile
#	Makefile
#	README.md
#	activation/activation.go
#	activation/activation_test.go
#	activation/activationdb_test.go
#	activation/atxdb.go
#	activation/fetch_mock.go
#	activation/nipst.go
#	activation/nipst_test.go
#	activation/poetdb.go
#	activation/poetlistener_test.go
#	activation/validator.go
#	api/api_test.go
#	api/grpcserver/debug_service.go
#	api/grpcserver/gateway_service.go
#	api/grpcserver/globalstate_service.go
#	api/grpcserver/grpcserver_test.go
#	api/grpcserver/http_server.go
#	api/grpcserver/mesh_service.go
#	api/grpcserver/node_service.go
#	api/grpcserver/smesher_service.go
#	api/node.go
#	blocks/blockeligibilityvalidator.go
#	blocks/blockeligibilityvalidator_test.go
#	blocks/blockoracle.go
#	blocks/blockoracle_test.go
#	blocks/blocks.go
#	blocks/blocks_test.go
#	cmd/hare/oracle.go
#	cmd/node/app_test.go
#	cmd/node/multi_node.go
#	cmd/node/node.go
#	cmd/node/node_test.go
#	cmd/node/oracle.go
#	cmd/p2p/p2p.go
#	cmd/root.go
#	cmd/sync/sync.go
#	cmd/sync/sync_test.go
#	collector/memory_collector.go
#	common/types/activation.go
#	common/types/block.go
#	config/config.go
#	eligibility/fixedoracle.go
#	eligibility/fixedoracle_test.go
#	fetch/fetch.go
#	fetch/fetch_test.go
#	go.mod
#	go.sum
#	hare/algorithm.go
#	hare/algorithm_test.go
#	hare/broker.go
#	hare/eligibility/beacon_test.go
#	hare/eligibility/oracle.go
#	hare/eligibility/oracle_test.go
#	hare/flows_test.go
#	hare/hare.go
#	hare/messagevalidation.go
#	hare/messagevalidation_test.go
#	hare/mock_oracle.go
#	hare/mock_oracle_test.go
#	hare/preroundtracker.go
#	hare/preroundtracker_test.go
#	hare/proposaltracker.go
#	hare/refcounttracker.go
#	layerfetcher/layers.go
#	layerfetcher/layers_test.go
#	log/log.go
#	mesh/mesh.go
#	mesh/mesh_test.go
#	mesh/meshdb.go
#	mesh/meshdb_test.go
#	mesh/reward_test.go
#	miner/block_builder.go
#	miner/builder_test.go
#	p2p/connectionpool/connectionpool_test.go
#	p2p/discovery/getaddress.go
#	p2p/discovery/ping.go
#	p2p/gossip/protocol.go
#	p2p/net/network.go
#	p2p/net/udp.go
#	p2p/server/msgserver.go
#	p2p/server/msgserver_test.go
#	p2p/service/gossip_listener.go
#	p2p/service/gossip_listener_test.go
#	p2p/service/service.go
#	p2p/service/sim.go
#	p2p/switch.go
#	p2p/switch_test.go
#	state/processor.go
#	sync/block_listener.go
#	sync/block_listener_test.go
#	sync/create_baseline_test.go
#	sync/fetch_queue.go
#	sync/fetch_queue_test.go
#	sync/handler.go
#	sync/mocks.go
#	sync/requests.go
#	sync/syncer.go
#	sync/syncer_test.go
#	sync/validation_queue.go
#	sync/worker.go
#	sync/worker_test.go
#	tests/Pipfile
#	tests/README.md
#	tests/block_atx/add_node/config.yaml
#	tests/block_atx/remove_node/config.yaml
#	tests/config.py
#	tests/config.yaml
#	tests/conftest.py
#	tests/deployment.py
#	tests/elk/elastic/elastic_logging.yaml
#	tests/elk/elastic/elastic_stateful.yaml
#	tests/elk/fluent-bit/config.yaml
#	tests/elk/fluent-bit/fluentbit_daemonset.yaml
#	tests/elk/kibana/kibana_logging.yaml
#	tests/elk/logstash/pipeline-config.yml
#	tests/elk/logstash/statefulset.yaml
#	tests/k8s_handler.py
#	tests/late_nodes/delayed_config.yaml
#	tests/node_pool_deployer.py
#	tests/p2p/bootstrap_test/config.yaml
#	tests/p2p/test_p2p.py
#	tests/stress/blocks_stress/config.yaml
#	tests/stress/grpc_stress/config.yaml
#	tests/stress/tx_stress/config.yaml
#	tests/sync/config.yaml
#	tests/sync/test_sync.py
#	tests/tx_generator/config.yaml
#	tests/utils.py
#	timesync/ticker.go
#	tortoise/util.go
#	tortoise/verifying_algorithm.go
#	tortoise/verifying_tortoise.go
#	tortoise/verifying_tortoise_test.go
@moshababo
Copy link
Contributor Author

bors merge

@bors
Copy link

bors bot commented Jul 26, 2021

👎 Rejected by code reviews

common/types/activation.go Show resolved Hide resolved
@moshababo
Copy link
Contributor Author

bors merge

@bors
Copy link

bors bot commented Jul 26, 2021

👎 Rejected by code reviews

@moshababo
Copy link
Contributor Author

bors merge

bors bot pushed a commit that referenced this pull request Jul 26, 2021
Implemented the new [Smesher API](https://github.com/spacemeshos/api/blob/master/proto/spacemesh/v1/smesher.proto) (instead of the previous Mining API), while integrating GPU/Tree-free PoST.
This PR is based on spacemeshos/post#41, which need to be reviewed first, hence this description won't repeat what was already been specified there.

Main remarks:

* Created `PostSetupManager` for implementing the new Post setup API. The previous `PostClient` was removed, and setup functionalities previously implemented by the ATX builder were removed as well. 
* Refactored the ATX builder to comply with the new Smeshing API. Its dependency with `PostSetupManager` is only regarding the setup completion status and generation of proofs. 
* In addition to the raw Post, the NIPost now contains the Post metadata as well, that the proof is expected to be verified upon during the syntactic validation. Some of the metadata values are expected to be derived from consensus, hence should be validated in the contextual validation, but it is currently still being derived from the node configuration, so it’s still taking place in the syntactic validation (which will need to remain anyway for validating min/max boundaries).
* Added `PostSetupOpts` config. It is used for initiating a new data creation session - either via the Smesher API, or on node start. 
* Renamed the ATX’s `commitment` as `initialPost`. As discussed with @tal-m, in the tree-free PoST the proof is *not* a commitment to the data.
* Changed NIPoST’S PoET challenge to contain the initial Post indices, instead of its Merkle root, which is no longer applicable. The reason is the same - it is expected to be known at the time the challenge was constructed. 
* Renamed “nipst” as “NIPost” throughout the codebase (closing #1371).
* Some of the old API code and test-code was commented out or disabled due to compile-time errors caused to the new defined interfaces. It is expected to be fully removed soon. 



Co-authored-by: Lane Rettig <lanerettig@gmail.com>
Co-authored-by: Alexey Sudachen <alexey@sudachen.name>
Co-authored-by: Noam Nelke <noam@spacemesh.io>
@bors
Copy link

bors bot commented Jul 26, 2021

Pull request successfully merged into develop.

Build succeeded:

@bors bors bot changed the title Smesher API + GPU/Tree-free PoST integration [Merged by Bors] - Smesher API + GPU/Tree-free PoST integration Jul 26, 2021
@bors bors bot closed this Jul 26, 2021
@bors bors bot deleted the smesher_api branch July 26, 2021 20:10
bors bot pushed a commit that referenced this pull request Aug 12, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 13, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 14, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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 pull request Aug 14, 2021
## Motivation
Nodes in app test used to have exponentially different weights. This was removed in #2149, and all nodes currently have the same weight. The test is more realistic if nodes have very different weights.

## Changes
- add back differing weights

## Test Plan
N/A, existing app tests should pass

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [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)
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.

None yet

6 participants