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] - Extend proposal builder to allow the use of a fallback active set. #5378

Closed
wants to merge 6 commits into from

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Dec 19, 2023

Motivation

related to #5366

Allow to set a fallback active set to be used by the proposal builder via the same mechanism as we use for the fallback beacon.

Changes

  • some minor fixes in bootstrap cmd code
  • extend ProposalBuilder to be able to set a specific active set for an epoch
  • extend node routine listening for updates to fallback beacon / activeset to propagate a fallback to the ProposalBuilder

Test Plan

  • added test case to ProposalBuilder to verify use of fallback active set if available
  • manual testing on testnet-10

TODO

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

@fasmat fasmat self-assigned this Dec 19, 2023
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (c580fba) 77.3% compared to head (44d50ce) 77.3%.

Files Patch % Lines
node/node.go 26.3% 14 Missing ⚠️
miner/proposal_builder.go 89.8% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #5378     +/-   ##
=========================================
- Coverage     77.3%   77.3%   -0.1%     
=========================================
  Files          258     258             
  Lines        30526   30590     +64     
=========================================
+ Hits         23599   23648     +49     
- Misses        5429    5444     +15     
  Partials      1498    1498             

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

node/node.go Outdated Show resolved Hide resolved
miner/proposal_builder.go Outdated Show resolved Hide resolved
miner/proposal_builder.go Show resolved Hide resolved
miner/proposal_builder.go Outdated Show resolved Hide resolved
@@ -368,6 +373,20 @@ func (pb *ProposalBuilder) decideMeshHash(ctx context.Context, current types.Lay
return mesh
}

func (pb *ProposalBuilder) UpdateActiveSet(epoch types.EpochID, activeSet []types.ATXID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about also providing weight as a part of fallback data?

Copy link
Member Author

@fasmat fasmat Dec 19, 2023

Choose a reason for hiding this comment

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

We could, I wanted to avoid that because for this we have to change the schema of the json thats inside the bootstrap file; to change it in a backwards compatible way the weight would then be optional.

@dshulyak
Copy link
Contributor

Should the activeset be persisted in the activeset table when received via update?

yes we should persist it on every node, otherwise non-smeshing nodes will ask it over the p2p network

Is it correct to fetch all missing ATXs from the fallback active set immediatly?

it is better to drop it from this pr, it is covered in my change.

what about sharing weight as well? it would make this hack more robust

@fasmat
Copy link
Member Author

fasmat commented Dec 19, 2023

yes we should persist it on every node, otherwise non-smeshing nodes will ask it over the p2p network

Will add that.

it is better to drop it from this pr, it is covered in my change.

I saw 🙂 for the test build I removed my code and used the code from your PR.

what about sharing weight as well? it would make this hack more robust

we could; I wasn't sure about it because it will force all nodes to update then (even for beacon).

@dshulyak
Copy link
Contributor

we could; I wasn't sure about it because it will force all nodes to update then (even for beacon).

do you mean that if we will add one new field, old versions will reject data with that field?

@fasmat
Copy link
Member Author

fasmat commented Dec 19, 2023

do you mean that if we will add one new field, old versions will reject data with that field?

I didn't try, just adding the field probably works. We can also add it to the schema if we don't make it a required field (to not break backwards compatibility). At the moment the only required fields are version and epoch, all other fields are optional.

fasmat added a commit that referenced this pull request Dec 20, 2023
fasmat added a commit that referenced this pull request Dec 20, 2023
@fasmat
Copy link
Member Author

fasmat commented Dec 21, 2023

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Dec 21, 2023
…5378)

## Motivation
related to #5366 

Allow to set a fallback active set to be used by the proposal builder via the same mechanism as we use for the fallback beacon.

## Changes
- some minor fixes in `bootstrap` cmd code
- extend `ProposalBuilder` to be able to set a specific active set for an epoch
- extend node routine listening for updates to fallback beacon / activeset to propagate a fallback to the `ProposalBuilder`

## Test Plan
- added test case to `ProposalBuilder` to verify use of fallback active set if available
- manual testing on `testnet-10`

## 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
- [x] Update documentation as needed
- [x] Update [changelog](../CHANGELOG.md) as needed
fasmat added a commit that referenced this pull request Dec 21, 2023
@spacemesh-bors
Copy link

Build failed:

  • systest-status

@fasmat
Copy link
Member Author

fasmat commented Dec 21, 2023

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Dec 21, 2023
…5378)

## Motivation
related to #5366 

Allow to set a fallback active set to be used by the proposal builder via the same mechanism as we use for the fallback beacon.

## Changes
- some minor fixes in `bootstrap` cmd code
- extend `ProposalBuilder` to be able to set a specific active set for an epoch
- extend node routine listening for updates to fallback beacon / activeset to propagate a fallback to the `ProposalBuilder`

## Test Plan
- added test case to `ProposalBuilder` to verify use of fallback active set if available
- manual testing on `testnet-10`

## 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
- [x] Update documentation as needed
- [x] Update [changelog](../CHANGELOG.md) as needed
@spacemesh-bors
Copy link

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title Extend proposal builder to allow the use of a fallback active set. [Merged by Bors] - Extend proposal builder to allow the use of a fallback active set. Dec 21, 2023
@spacemesh-bors spacemesh-bors bot closed this Dec 21, 2023
@spacemesh-bors spacemesh-bors bot deleted the fetch-active-set branch December 21, 2023 14:10
dsmello pushed a commit that referenced this pull request Dec 28, 2023
…5378)

## Motivation
related to #5366 

Allow to set a fallback active set to be used by the proposal builder via the same mechanism as we use for the fallback beacon.

## Changes
- some minor fixes in `bootstrap` cmd code
- extend `ProposalBuilder` to be able to set a specific active set for an epoch
- extend node routine listening for updates to fallback beacon / activeset to propagate a fallback to the `ProposalBuilder`

## Test Plan
- added test case to `ProposalBuilder` to verify use of fallback active set if available
- manual testing on `testnet-10`

## 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
- [x] Update documentation as needed
- [x] Update [changelog](../CHANGELOG.md) as needed
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.

2 participants