-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add Electrum backend #1222
Add Electrum backend #1222
Conversation
27367e1
to
0af1a78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just been through the commits without reading the details. Thanks for neatly organizing the PR, it's a pleasure to read through.
Just a few drive-by comments, nothing major.
f27ed53
to
0ef4613
Compare
0ef4613
to
ea66f79
Compare
I've updated the |
a3cea58
to
bb0b981
Compare
I've made various fixes after running the functional tests locally:
I'm now preparing the updates to functional tests so that they use Electrum as a backend and will push these changes when ready.
As discussed in Discord, I don't think we need the rescan functionality when using the Electrum backend given that a full scan is performed at wallet creation (for both new & imported wallets). The
|
bb0b981
to
dac26c8
Compare
I've made a further fix to how unconfirmed transactions are handled. It was possible before, especially when running the functional tests, that two polls would be done within the same second and so setting the last seen to the current timestamp would not identify those unconfirmed transactions that had dropped from the mempool. I've now added a I also fixed an error in how the wallet's coins were returned in case a coin's spend txid had dropped from the mempool. It was previously dropping the whole coin whereas now I only drop the spend txid value and keep the coin. I've updated the functional tests to work with Electrum (specifically electrs). The next step is to add this electrs backend to the CI pipeline. |
47ad084
to
42710e4
Compare
I've now added the Electrum backend to the CI pipeline. There's a failing test but I think it's just a transient error. |
Unrelated to the Electrum changes, I added the test_spend.py tests to the CI as I don't think there was any reason for them not to be included. The failing test is passing on my local machine and seems to be a transient error. |
This change will be added as part of #1235 instead. |
a9d1d88
to
f87912f
Compare
Rebased on #1235. |
This includes changes from darosior's comment in wizardsardine#1222 (comment).
This is copied from darosior's changes in wizardsardine#1222 (comment).
20c1880
to
f6c54f9
Compare
Rebased on master and applied @darosior's rescan logic. |
@jp1ac4 to fix the functional tests: diff --git a/tests/test_framework/electrs.py b/tests/test_framework/electrs.py
index 1dc12d3..ee8c6a5 100644
--- a/tests/test_framework/electrs.py
+++ b/tests/test_framework/electrs.py
@@ -19,6 +19,10 @@ class Electrs(BitcoinBackend):
if rpcport is None:
rpcport = reserve()
+ # Prometheus metrics can't be deactivated in Electrs. Configure the port so it doesn't
+ # conflict with other instances when running tests in parallel.
+ monitoring_port = reserve()
+
self.electrs_dir = electrs_dir
self.rpcport = rpcport
@@ -39,6 +43,7 @@ class Electrs(BitcoinBackend):
"db_dir": electrs_dir,
"network": "regtest",
"electrum_rpc_addr": f"127.0.0.1:{self.rpcport}",
+ "monitoring_addr": f"127.0.0.1:{monitoring_port}",
}
self.conf_file = os.path.join(regtestdir, "electrs.toml")
with open(self.conf_file, "w") as f: h/t @pythcoiner for pointing to me it's what conflicted between instances. |
This is copied from darosior's changes in wizardsardine#1222 (comment).
f6c54f9
to
6901f8e
Compare
Applied these changes and another small fix to do with checking the backend type. There's an issue with electrs returning an error when a channel it's sending on becomes disconnected (which happens in some tests when we stop lianad). To make the tests pass, we could restart the electrs backend in that case, but this wouldn't address the underlying issue. |
run only
Full logs:
|
Applied @pythcoiner's fix for the failing functional tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 79ddfbb. Didn't go through everything in details once again, but it's certainly good enough to start testing along with the other features in master.
@@ -107,8 +106,9 @@ def restart_fresh(self, bitcoind): | |||
self.stop() | |||
dir_path = os.path.join(self.datadir, "regtest") | |||
shutil.rmtree(dir_path) | |||
wallet_path = os.path.join(dir_path, "lianad_watchonly_wallet") | |||
bitcoind.node_rpc.unloadwallet(wallet_path) | |||
if BITCOIN_BACKEND_TYPE is BitcoinBackendType.Bitcoind: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could have used self.backend
rather than the constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would work as self.backend
is of type BitcoinBackend
rather than BitcoinBackendType
.
Another option would be to add a backend_type()
method to the BitcoinBackend
abstract class so we could use if self.backend.backend_type() is BitcoinBackendType.Bitcoind
.
) -> Result<Electrum, StartupError> { | ||
let electrum_config = match config.bitcoin_backend.as_ref() { | ||
Some(config::BitcoinBackend::Electrum(electrum_config)) => electrum_config, | ||
_ => Err(StartupError::MissingElectrumConfig)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it could be asserted since this function is only ever called when this condition is true.
This includes changes from darosior's comment in wizardsardine#1222 (comment).
Here, the min RBF feerate is 1 more than that of the transaction to be replaced. The feerate of the transaction to be replaced may vary slightly depending on the signature size and is calculated during the test. As such, the min feerate should be set according to this calculated value.
This is copied from darosior's changes in wizardsardine#1222 (comment).
Thanks to pythcoiner for providing this fix.
79ddfbb
to
341f940
Compare
re-ACK 341f940 |
The reorg tests flakiness demonstrated by CI will be addressed in a followup. @jp1ac4 will open an issue for this. |
819eb92 gui(settings): allow to change node type (Michael Mallan) 2381227 gui(settings): view & edit Electrum settings (Michael Mallan) b570039 gui(settings): rename Bitcoin Core to Node (Michael Mallan) db20ae4 gui(installer): reduce empty space height (Michael Mallan) 0993905 gui(installer): update wording to include Electrum (Michael Mallan) f40af57 gui(installer): split long string and run cargo fmt (Michael Mallan) 0f09be1 gui(installer): don't change values while waiting for ping result (Michael Mallan) c93aa88 gui(installer): add electrum node option (Michael Mallan) 341e446 gui(installer): allow for different node types (Michael Mallan) 83172c7 gui(installer): add general node definition (Michael Mallan) 046b54e gui(installer): define bitcoind from general node struct (Michael Mallan) c5d9d00 gui: move bitcoind to new node module (Michael Mallan) 4536eff gui(installer): extract logic for try ping bitcoind (Michael Mallan) ef44cf3 gui(installer): add module for node step (Michael Mallan) f74f071 gui: upgrade liana dependency (jp1ac4) Pull request description: This is for #1223. For now, it's possible to edit the node's settings but not to change node type. Remaining tasks: - [x] Revert Cargo.toml once #1222 is merged. - [x] Update wording as per #1223 (comment). ACKs for top commit: pythcoiner: ACK 819eb92 Tree-SHA512: 362a14d32c2e13ba286d252d9f8a1106d63e5c40198776653b0623b433435329663126307e17da017fdbbd8a8ad273b703cc3ba54fd13fa5a0afd7dd9179089a
This is to add the Electrum backend as part of #56 (comment).
This requires the database to be running version 5 following #1180. The migration from a previous DB version must be done using bitcoind. Thereafter, the daemon can be run using an Electrum backend by replacing the
[bitcoind_config]
section in the daemon.toml config file with:Remaining tasks:
MempoolEntry