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

Fix typo in nakamoto tenures table #4812

Merged
merged 9 commits into from
May 28, 2024

Conversation

ASuciuX
Copy link
Contributor

@ASuciuX ASuciuX commented May 21, 2024

Description

  • fix cause typo
  • add checker strict
  • add drop table

Applicable issues

@ASuciuX ASuciuX requested a review from a team as a code owner May 21, 2024 18:30
@ASuciuX ASuciuX self-assigned this May 21, 2024
@ASuciuX ASuciuX linked an issue May 21, 2024 that may be closed by this pull request
@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 21, 2024

Hey @kantai, @obycode, do we need to add an extra if statement for the drop, something like drop table only if column schema-version doesn't exist?

Also, how could I test this? I ran RUST_BACKTRACE=full STACKS_LOG_DEBUG=1 BITCOIND_TEST=1 cargo test -- --test-threads 1 and it failed, but also the develop branch from which I've started failed the same way.

thread 'tests::should_succeed_mining_valid_txs' panicked at testnet/stacks-node/src/node.rs:339:10:
FATAL: failed to initiate mempool: Other("Failed to open chainstate: ClarityError(Interpreter(Interpreter(MarfFailure(\"unable to open database file: /tmp/stacks-node-tests/integrations-neon/68456e5053d7c4e4-1716316920/mocknet/chainstate/vm/clarity/marf.sqlite\"))))")

Copy link
Member

@kantai kantai 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 this @ASuciuX

Yes, this schema update needs to be controlled via the schema versioning.

You should:

  1. Rename NAKAMOTO_TENURES_SCHEMA to NAKAMOTO_TENURES_SCHEMA_1
  2. Create a new NAKAMOTO_TENURES_SCHEMA_2 which includes the DROP TABLE and the new CREATE TABLE.
  3. Create a NAKAMOTO_CHAINSTATE_SCHEMA_2 variable in stackslib/src/chainstate/nakamoto/mod.rs which includes the NAKAMOTO_TENURES_SCHEMA_2
  4. Update CHAINSTATE_VERSION to 5 in stackslib/src/chainstate/stacks/db/mod.rs
  5. Add a new "4" case to the match in apply_schema_migrations in stackslib/src/chainstate/stacks/db/mod.rs, which applies NAKAMOTO_CHAINSTATE_SCHEMA_2.

@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 22, 2024

Thank you very much @kantai! How could I test what I added so far?
Update from Jude:

  1. Add test adding only the first schema to see it exists with the problem.
  2. Check it exists with typo as expected.
  3. Add the fix typo schema_2 and check it updated as expected.

I also looked over other tables and I spotted here that on one table (different table than the one this issue is pointing to), some fields are declared as INT and others as INTEGER? I've looked through the docs and saw they both result in INTEGER. If we consider adding the STRICT tag to it theoretically it shouldn't result in the affinity as INT it's not a 1-on-1 match with INTEGER. [doc link] .

Update: with strict it can still be INT or INTEGER doc link. They should be the same so what is the approach on creating a column with INT vs INTEGER in a table that has both of them?

@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 27, 2024

According to the docs, the first sqlite_version that supports STRICT is 3.37.0. From the stacks-core, the sqlite_version is 3.33.0 and the tests were failing with this output:

Migrating chainstate schema from version 4 to 5: fix nakamoto tenure typo
thread 'chainstate::nakamoto::tests::test_make_miners_stackerdb_config' panicked at stackslib/src/net/mod.rs:2452:14:
called `Result::unwrap()` on an `Err` value: DBError(SqliteError(SqliteFailure(Error { code: Unknown, extended_code: 1 }, Some("near \"STRICT\": syntax error"))))

I would remove from this issue the STRICT and add it after we update the sqlite_version.

After removing the STRICT from the table creation, the tests passed ( with the STRICT 3 tests failed).

@ASuciuX ASuciuX requested a review from a team as a code owner May 27, 2024 19:25
Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.89%. Comparing base (66f9883) to head (7b423bd).
Report is 9 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4812      +/-   ##
===========================================
- Coverage    83.49%   76.89%   -6.61%     
===========================================
  Files          481      481              
  Lines       344880   344924      +44     
  Branches       323      323              
===========================================
- Hits        287953   265218   -22735     
- Misses       56919    79698   +22779     
  Partials         8        8              
Files Coverage Δ
stacks-signer/src/signerdb.rs 89.47% <100.00%> (-2.00%) ⬇️
stackslib/src/chainstate/nakamoto/mod.rs 82.18% <ø> (-1.33%) ⬇️
stackslib/src/chainstate/nakamoto/signer_set.rs 81.90% <ø> (-0.26%) ⬇️
stackslib/src/chainstate/nakamoto/tenure.rs 86.91% <ø> (ø)
stackslib/src/chainstate/stacks/db/mod.rs 84.21% <100.00%> (-0.02%) ⬇️

... and 203 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@ASuciuX ASuciuX requested a review from kantai May 28, 2024 14:34
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!

@ASuciuX ASuciuX enabled auto-merge May 28, 2024 19:07
@ASuciuX ASuciuX added this pull request to the merge queue May 28, 2024
Merged via the queue into stacks-network:develop with commit 969f4a2 May 28, 2024
1 of 2 checks passed
obycode added a commit that referenced this pull request May 30, 2024
This should have been included in #4812 but was overlooked. The fix
solves the issue with the various Nakamoto integration tests.
obycode added a commit that referenced this pull request May 30, 2024
This should have been included in #4812 but was overlooked. The fix
solves the issue with the various Nakamoto integration tests.
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Fix typo in nakamoto_tenures table
4 participants