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.

Sign up for GitHub

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

Jump to bottom

Feat/chain-id #3111

Merged
merged 29 commits into from
Apr 27, 2022
Merged

Feat/chain-id #3111

merged 29 commits into from
Apr 27, 2022

Conversation

jcnelson
Copy link
Member

This PR address #2982 by adding chain-id as a global variable in Clarity2, which resolves to StacksChainState::chain_id.

@jcnelson
feat: add chain-id global variable, which evaluates to the chain_id…
f8c824a 
… value in GlobalContext (which in turn gets it from the ClarityInstance, from StacksChainState)
@jcnelson
chore: add unit tests for chain-id variable
450039b
@jcnelson
chore: plumb through chain_id into ClarityInstance and its dependant …
16da7ba 
…structs for block and transaction connections
@codecov
Copy link

codecov bot commented Apr 22, 2022 •
edited
Loading

Codecov Report

Merging #3111 (2747901) into next (e9f7949) will decrease coverage by 0.21%.
The diff coverage is 83.38%.

@@            Coverage Diff             @@
##             next    #3111      +/-   ##
==========================================
- Coverage   83.97%   83.75%   -0.22%     
==========================================
  Files         265      265              
  Lines      207719   207954     +235     
==========================================
- Hits       174426   174166     -260     
- Misses      33293    33788     +495
Impacted Files Coverage Δ
clarity/src/vm/analysis/type_checker/mod.rs 93.69% <0.00%> (-0.15%) ⬇️
src/core/mod.rs 76.84% <ø> (ø)
src/util_lib/db.rs 82.28% <0.00%> (-0.57%) ⬇️
stacks-common/src/libcommon.rs 100.00% <ø> (ø)
src/chainstate/burn/db/sortdb.rs 95.22% <40.00%> (-0.25%) ⬇️
testnet/stacks-node/src/run_loop/neon.rs 80.00% <53.33%> (-0.65%) ⬇️
src/chainstate/coordinator/mod.rs 90.67% <55.55%> (-1.73%) ⬇️
src/clarity_cli.rs 76.80% <66.66%> (-0.08%) ⬇️
src/clarity_vm/tests/large_contract.rs 81.19% <66.66%> (ø)
clarity/src/vm/analysis/arithmetic_checker/mod.rs 92.64% <100.00%> (ø)
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2af13c...2747901. Read the comment docs.

@jcnelson
fix: add separate DB migration function so the chains coordinator can…
cb6861d 
… do it on node boot-up. Also, downgrade the tx type for migraiton to DBTx so we don't accidentally depend on the first block header information (which we might not have)
@jcnelson
feat: add function to check and migrate chainstate DBs if they exist
485e450
@jcnelson
fix: an always-allowed peer is already authenticated since we have it…
dfc4c49 
…s public key, so also make sure that the last contact time is positive when considering timeouts
@jcnelson
chore: add OldSchema(..) and TooOldForEpoch error variants for schema…
32741c8 
… migrations
@jcnelson
fix: update the public key of an always-allowed peer on boot-up, as w…
889bc9b 
…ell as the `allowed` column
@jcnelson
fix: if the chainstate DBs exist already, then they must be migrated …
8f472cf 
…to the latest schema before header sync
gregorycoppola
gregorycoppola approved these changes Apr 22, 2022
View reviewed changes
clarity/src/vm/tests/simple_apply_eval.rs Outdated
execute_with_parameters(
program,
ClarityVersion::Clarity2,
StacksEpochId::Epoch20,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not Epoch21? same as below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's an oversight on my end. Will fix.

@jcnelson jcnelson changed the base branch from chore/merge-develop-to-next to next April 26, 2022 15:44
obycode
obycode reviewed Apr 27, 2022
View reviewed changes
clarity/src/vm/contexts.rs Show resolved Hide resolved
src/clarity_vm/tests/analysis_costs.rs Outdated Show resolved Hide resolved
@jcnelson jcnelson requested a review from obycode April 27, 2022 16:57
obycode
obycode reviewed Apr 27, 2022
View reviewed changes
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Thanks for that change with test_only_mainnet_to_chain_id.

clarity/src/vm/costs/mod.rs Outdated Show resolved Hide resolved
clarity/src/vm/contexts.rs Outdated Show resolved Hide resolved
clarity/src/vm/mod.rs Outdated Show resolved Hide resolved
src/clarity_vm/tests/analysis_costs.rs Outdated Show resolved Hide resolved
src/clarity_vm/tests/events.rs Outdated Show resolved Hide resolved
obycode
obycode approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM!

@jcnelson jcnelson merged commit 9407d62 into next Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obycode obycode obycode approved these changes

@gregorycoppola gregorycoppola gregorycoppola approved these changes

@kantai kantai Awaiting requested review from kantai

@lgalabru lgalabru Awaiting requested review from lgalabru

Assignees
No one assigned
Labels
stacks-2.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jcnelson @obycode @gregorycoppola @kantai