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

Check core version before attempting inscription. #1048

Merged
merged 15 commits into from
Dec 31, 2022

Conversation

rot13maxi
Copy link
Contributor

Example of the error that it throws:

error: Bitcoin Core version 240000 or greater required. Current Bitcoin Core reports version 230000

the current release 24.0.1 reports as 240001.

closes #1045

@casey
Copy link
Collaborator

casey commented Dec 23, 2022

Nice! Don't worry about rebasing, I always just squash-merge in the end, and can ping you if there are conflicts that I can't resolve.

This could use an integration test. There is a mock Bitcoin Core API server, in test-bitcoincore-rpc, that we run tests against. The tests are currently failing, because we return 230000 in getnetworkinfo, which must be what bitcoincore-rpc uses to figure out the version.

Check out tests/wallet.rs for some examples.

To make things less annoying, I pushed a PR adding a builder to the test-bitcoincore-rpc crate to this PR. So to add the test you can:

  • Create a new test in tests/wallet.rs
  • Build a test-bitcoincore-rpc server using the builder that sets the version to less than 240000
  • Make sure you get an error

Additionally, the version numbers that core returns are pretty unintelligible, so we should fix them before reporting them to the user. Something like:

fn format_bitcoin_core_version(version: usize) -> String {
  format!(
    "{}.{}.{}",
    version / 10000,
    version % 10000 / 100,
    version % 100
  )
}

@rot13maxi
Copy link
Contributor Author

great feedback, thanks!

@casey
Copy link
Collaborator

casey commented Dec 23, 2022

Whoops, I think I jumped the gun on reviewing this. I pushed a diff formatting everything. I think your editor must have auto-formatted without consulting rustfmt.toml.

@casey
Copy link
Collaborator

casey commented Dec 31, 2022

I think you accidentally nuked the via force-push the commit I added with the builder, so I resurrected it.

@casey casey enabled auto-merge (squash) December 31, 2022 07:58
@casey casey merged commit a2394a5 into ordinals:master Dec 31, 2022
@casey
Copy link
Collaborator

casey commented Dec 31, 2022

Wanted to get this in, since I think it's a common error!

@rot13maxi
Copy link
Contributor Author

been out of town for a bit and hadn't touched this. thanks for the commits!

@rot13maxi rot13maxi deleted the version-checl branch December 31, 2022 18:26
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.

Check bitcoin core version when inscribing
2 participants