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

SPV multi-server cross-validation #59

Merged
merged 37 commits into from Mar 2, 2021

Conversation

shesek
Copy link
Contributor

@shesek shesek commented Oct 19, 2020

Also includes a bunch of other small changes/fixups.

This PR depends on:

@shesek shesek force-pushed the 202010-spv-crossvalidation branch 2 times, most recently from 9d0d798 to f91b18d Compare November 3, 2020 11:40
@shesek
Copy link
Contributor Author

shesek commented Nov 4, 2020

I added a new SPVVerifyResult::NotLongest status. Its encoded as the string not_longest or as the number 4. When spv cross-validation fails, all of the transactions will report this status.

Should this status also be exposed as a global flag of some sort, or is having this on the individual transactions sufficient?

The cross-validator reports some information about the chain fork, including how far behind the primary electrum server is compared to the longest chain (both work and height wise). Should this information be made available to the app?

@shesek shesek force-pushed the 202010-spv-crossvalidation branch 4 times, most recently from 96d3328 to e8729bc Compare November 12, 2020 14:07
@shesek
Copy link
Contributor Author

shesek commented Nov 14, 2020

I added a script for updating the Electrum servers list from the latest Electrum release: https://github.com/shesek/gdk/blob/202010-spv-crossvalidation/subprojects/gdk_rust/gdk_electrum/scripts/update-server-list.sh

Should this be added to some build process somewhere?

@shesek
Copy link
Contributor Author

shesek commented Dec 17, 2020

Rebased on master and removed the dependency on my rust-electrum-client fork (the necessary changes were added upstream and released in v0.4.0). It still depends on my rust-bitcoin fork pending a PR I sent there.

Other standing items/questions:

  • The apps need to handle the new NotLongest and Unconfirmed SPV verification statuses, as well as support configuring the new spv_cross_validation and spv_cross_validation_servers options.

  • Should the SPV cross-validation status be exposed as global session state, in addition to being included on individual transactions?

    This could be useful for displaying cross-validation failures somewhere prominent (outside of individual txs), and could also provide the app some additional details about the chain fork, like the local height, the height of the best known chain and the additional work in the best chain.

  • Cross-validation is still not implemented in the C FFI used by the Green apps for SPV verification. In that environment, the Rust side doesn't run background threads of its own, it only runs when invoked from the CPP side. This introduces some complexities, both because cross-validation is an ongoing process and because the secondary servers are likely to be slow (or entirely unresponsive), which could result in user-visible latency if its not done in a separate thread.

@LeoComandini
Copy link
Contributor

LeoComandini commented Dec 23, 2020

Rebased on master and removed the dependency on my rust-electrum-client fork (the necessary changes were added upstream and released in v0.4.0). It still depends on my rust-bitcoin fork pending a PR I sent there.

Other standing items/questions:

* The apps need to handle the new `NotLongest` and `Unconfirmed` SPV verification statuses, as well as support configuring the new `spv_cross_validation` and `spv_cross_validation_servers` options.

* Should the SPV cross-validation status be exposed as global session state, in addition to being included on individual transactions?
  This could be useful for displaying cross-validation failures somewhere prominent (outside of individual txs), and could also provide the app some additional details about the chain fork, like the local height, the height of the best known chain and the additional work in the best chain.

* Cross-validation is still not implemented in the C FFI used by the Green apps for SPV verification. In that environment, the Rust side doesn't run background threads of its own, it only runs when invoked from the CPP side. This introduces some complexities, both because cross-validation is an ongoing process and because the secondary servers are likely to be slow (or entirely unresponsive), which could result in user-visible latency if its not done in a separate thread.

Currently no production app is using SPV verification. The points you raise are reasonable and will need to be addressed.
But getting the interface changes using by Green apps right it's not easy or fast. We must make sure those work and have acceptable performances with all supported apps and platform. Such process will involve engineers, testers and designers and will take a rather long period. Therefore the plan is to postpone interface decisions and review and merge these SPV cross validation changes even though they might be unusable or hard to use from Green sessions.

@LeoComandini
Copy link
Contributor

DEBUG=1 ./launch_integration_tests.sh bitcoin
DEBUG=1 ./launch_integration_tests.sh bitcoin

both fails with

thread 'bitcoin' panicked at 'assertion failed: `(left == right)`
  left: `"unconfirmed"`,
 right: `"in_progress"`', tests/test_session.rs:451:9

I guess they need updating

@LeoComandini
Copy link
Contributor

Please add to .gitlab-ci.yml

DEBUG=1 ./launch_integration_tests.sh spv_cross_validate
DEBUG=1 ./launch_integration_tests.sh spv_cross_validation_session

.gitlab-ci.yml Outdated Show resolved Hide resolved
@shesek shesek force-pushed the 202010-spv-crossvalidation branch 2 times, most recently from b5ff09b to d903e0b Compare December 23, 2020 12:03
@shesek
Copy link
Contributor Author

shesek commented Dec 23, 2020

I guess they need updating

Indeed, fixed in d903e0b.

@shesek
Copy link
Contributor Author

shesek commented Dec 23, 2020

Please add to .gitlab-ci.yml

Added in 31ebd84.

shesek and others added 15 commits February 27, 2021 06:20
- Don't use .onion servers (we currently don't support tor)
- Add the "noverify" flag to disable SSL certificate validation
- Allow non-SSL servers (doesn't matter in practice because Electrum's
  default server list doesn't have any)
It now has native support for setting the socket timeout,
implemented in bitcoindevkit/rust-electrum-client#27
Co-authored-by: LeoComandini <26792912+LeoComandini@users.noreply.github.com>
As expected by rust-electrum-client
The necessary changes have been merged, but still not released.
@LeoComandini
Copy link
Contributor

nit: typo "rust-elemenets" in last commit

@LeoComandini
Copy link
Contributor

Also, is this still WIP?

@shesek shesek force-pushed the 202010-spv-crossvalidation branch from 37925f0 to 6d47005 Compare March 1, 2021 23:48
- Introduced new BEScript, BETxid and BEBlockHash enums to encapsulate rust-elements's new data types.

- The new data types will BREAK the existing RawCache database, which will be automatically re-populated from the Electrum server. Care has been taken to retain compatibility with the RawStore database (containing the user's settings and tx labels).

- Added support for Signet.
@shesek shesek force-pushed the 202010-spv-crossvalidation branch from 6d47005 to c73f427 Compare March 2, 2021 00:29
@shesek shesek changed the title [WIP] SPV multi-server cross-validation SPV multi-server cross-validation Mar 2, 2021
@LeoComandini
Copy link
Contributor

re-ACK c73f427, review diff since last acked version, gitlab CI passed, manually tested that store data (settings and memos) are preserved after upgrade (0.0.40 to c73f427) and downgrade.

@LeoComandini LeoComandini merged commit c73f427 into Blockstream:master Mar 2, 2021
@LeoComandini
Copy link
Contributor

Thanks @shesek!

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

3 participants