Conversation
27367e1 to
0af1a78
Compare
darosior
left a comment
There was a problem hiding this comment.
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: Details |
|
Applied @pythcoiner's fix for the failing functional tests. |
| 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.
nit: could have used self.backend rather than the constant.
There was a problem hiding this comment.
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.
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 bitcoindevkit/bdk_wallet#127. 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