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

Firo light wallet (electrum) support #2426

Merged
merged 10 commits into from
Aug 31, 2023

Conversation

dev-warrior777
Copy link
Contributor

Firo light wallet (electrum) support

original pr

@dev-warrior777 dev-warrior777 marked this pull request as ready for review July 4, 2023 14:40
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, does this add an electrum server for simnet? That's great! Can we also do that for the other electrum capable wallets? Not in this pr ofc.

There is an empty file recycled-addrs.txt if something is creating that maybe add to .gitignore?

dex/testing/firo/electrum.sh Outdated Show resolved Hide resolved
dex/testing/firo/electrumx.sh Outdated Show resolved Hide resolved
@dev-warrior777
Copy link
Contributor Author

dev-warrior777 commented Jul 5, 2023

ElectrumX servers

Does this add an electrum server for simnet?

Yes, I think the dex/testing electrumX server code was mostly there or thereabouts for bitcoin but I did a Firo one as bitcoin electrum/electrumX is sufficiently different.

Can we also do that for the other electrum capable wallets?

I already did for Dash on my local machine and have my own fork of the server. But please look at the discussion of electrum Wallets below

Electrum wallets

I did the whole electrum piece because Firo does not yet have SPV wallet support (P2P) as it is lower on the priority list than Lelantus Spark and other multi year ZKP improvements. So an electrum 'SPV' wallet that would not require chain download would increase UX a lot! I would expect this to be the main (non OTC) wallet perhaps

Then I looked at all the other electrum-altcoin implementations.

  • I think BCH is possible. It was partly implemented in dex although currently commented out (client bch.go) I have run BCH one and there are electrumX servers but I did not get deep into the python yet although a quick glance shows some BCH-specific stuff in commands.py. However BCH already has (P2P) SPV so I guess not urgent/useful
  • Dash is kinda the opposite:
    • It is quite old. The original archived one is still python 2.7. Although it was updated to python3 and maintained in Ukraine. Those guys deleted the github totally and just have anti russian statements there. But it is maintained since last year by a semi-official fork of that code. The maintainer currently over busy though.
    • Another mainstream Dash dev has also just updated the C-python X11 hash algo for electrumX / electrum to work with python >= 3.10 so I think they still care.
    • But I see only one reliable mainnet Dash electrumX server - they had I think 5 some years back. So not sure the usage of Dash electrum today. I am working with one guy at Dash to research current usage / need for electrumX servers.
  • Doge .. not touched for 9 years ;-) doubt if it works, don't care also

@dev-warrior777
Copy link
Contributor Author

dev-warrior777 commented Jul 5, 2023

There is an empty file recycled-addrs.txt if something is creating that maybe add to

This is a function of another recent PR for recycling unused redemption and refund addresses and seems to create a directory where code is run and a file with recycled-addrs.txt. This happens when I run simnet tests in client/asset/... but I am not sure where else it would pop up (as testnet/mainnet/regtest) so not sure the extent of .gitignore

Including that particular one in my check-in is my bad and I will remove it ;-)

  • Removed

@dev-warrior777
Copy link
Contributor Author

Thanks for your review and your interest.

@JoeGruffins
Copy link
Member

Slightly unrelated, but testing litecoin's electrum client on testnet is an pain in the butt because the servers are all over the place on different states of the chain. Would be much better testing on simnet, so this pr is exciting for me. Will test this out soon.

@dev-warrior777
Copy link
Contributor Author

dev-warrior777 commented Jul 5, 2023

Slightly unrelated, but testing litecoin's electrum client on testnet is an pain in the butt because the servers are all over the place on different states of the chain. Would be much better testing on simnet, so this pr is exciting for me. Will test this out soon.

For Firo I have another test that tests basic ElectrumX command API. It tests one (real) mainnet server and one (real) testnet server plus simnet server on demand (set environment var) In the same place: dex/dcrdex/dex/testing/firo/test/electrumx_network_test.go. Again I updated a bitcoin test for this. You could test simnet server with server specific non-dex code as well as checking real servers

dex/testing/firo/test/README.md checked in

@dev-warrior777
Copy link
Contributor Author

dev-warrior777 commented Jul 5, 2023

Newer README's / scripts

@chappjc
Copy link
Member

chappjc commented Jul 5, 2023

Slightly unrelated, but testing litecoin's electrum client on testnet is an pain in the butt because the servers are all over the place on different states of the chain. Would be much better testing on simnet, so this pr is exciting for me. Will test this out soon.

LTC already had electrum(x) regtest stuff I think. What new?

@dev-warrior777
Copy link
Contributor Author

LTC already had electrum(x) regtest stuff I think. What new?

Yes. I copied it and just modify for Firo.
Bitcoin/LTC too different to put in same script

@JoeGruffins
Copy link
Member

JoeGruffins commented Jul 6, 2023

LTC already had electrum(x) regtest stuff I think. What new?

Oh! You are right. How did I miss that... a3dac7e

Hmm, I guess I knew and forgot. I'm getting old, srry #1607

dex/testing/firo/harness.sh Show resolved Hide resolved
dex/testing/firo/README_HARNESS.md Outdated Show resolved Hide resolved
dex/testing/firo/electrumx.sh Outdated Show resolved Hide resolved
dex/testing/firo/electrum.sh Show resolved Hide resolved
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well for me!

dex/testing/firo/README_ELECTRUM_HARNESSES.md Show resolved Hide resolved
client/asset/firo/firo.go Show resolved Hide resolved
dex/testing/firo/README_ELECTRUM_HARNESSES.md Outdated Show resolved Hide resolved
client/asset/firo/firo.go Outdated Show resolved Hide resolved
@dev-warrior777
Copy link
Contributor Author

Working well for me!

Great to hear!

client/asset/firo/firo.go Outdated Show resolved Hide resolved
client/asset/firo/firo.go Outdated Show resolved Hide resolved
client/core/simnet_trade.go Outdated Show resolved Hide resolved
dex/testing/firo/README_ELECTRUM_HARNESSES.md Outdated Show resolved Hide resolved
dex/testing/firo/README_ELECTRUM_HARNESSES.md Outdated Show resolved Hide resolved
dex/testing/firo/electrumx.sh Show resolved Hide resolved
dex/testing/firo/harness.sh Outdated Show resolved Hide resolved
dex/testing/firo/harness.sh Outdated Show resolved Hide resolved
dex/testing/firo/test/README.md Outdated Show resolved Hide resolved
dex/testing/firo/test/README.md Outdated Show resolved Hide resolved
@dev-warrior777
Copy link
Contributor Author

Added a compile check in run_tests.sh

...
go test -c -o /dev/null -tags harness ./client/asset/bch
go test -c -o /dev/null -tags harness ./client/asset/firo
go test -c -o /dev/null -tags harness ./client/asset/eth
...

client/asset/firo/firo.go Outdated Show resolved Hide resolved
client/asset/firo/firo.go Outdated Show resolved Hide resolved
client/asset/firo/firo.go Outdated Show resolved Hide resolved
dex/testing/firo/README_ELECTRUM_HARNESSES.md Outdated Show resolved Hide resolved
dex/testing/firo/README_ELECTRUM_HARNESSES.md Show resolved Hide resolved
dex/testing/firo/electrumx.sh Show resolved Hide resolved
dex/testing/firo/electrumx.sh Show resolved Hide resolved
dex/testing/firo/electrum.sh Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are GUI and daemon the only options. No standard terminal mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure what you mean by standard terminal mode. But I think the answer to your question is - only these two modes yes.

For startup the script is electrum-firo and --daemon takes it down the non-gui code path. But you can also run the script with something like:
./electrum-firo -v --offline --testnet listunspent
which will access the testnet wallet, ask password, then output all the listunspent utxos; then exit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm asking if we make electrum block and log to the terminal. I don't want it to run as a daemon.

dex/testing/firo/electrum.sh Show resolved Hide resolved
@dev-warrior777
Copy link
Contributor Author

dev-warrior777 commented Aug 26, 2023

I cannot find any of the chappjc requested changes below that I have not addressed. Maybe a github glitch?

@dev-warrior777
Copy link
Contributor Author

For your ref: Updated everything requested except the development zombie killer. Thanks

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on simnet and testnet. Working great. Almost ready.

client/asset/firo/firo.go Outdated Show resolved Hide resolved
Comment on lines 102 to 105
```bash
$ killall -9 firod # kill running daemons
$ tmux kill-session -t firo-harness # kill firo-harness session
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No pid file needed. There are a number of ways to figure out the pid of a zombie instance. People reading this document are expected to have some level of technical knowledge. We don't need to babysit here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm asking if we make electrum block and log to the terminal. I don't want it to run as a daemon.

$ ./run dcrfiroelectrum --runonce --all
```

Best results by nuking everything after each run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our various scripts should be nuking everything during setup, no?

@dev-warrior777
Copy link
Contributor Author

I have updated the livetest code & updated the docs.

For your questions:

Our various scripts should be nuking everything during setup, no?

When the harnesses start Yes
What I mean't was don't run the simnet-trade-test suite twice with the same harnesses. They can run out of coins. Any on the fly restarts of one harness can make harnesses not in sync w.r.t underlying regtest/simnet chain blocks, transactions, height, etc. This is especially true for electrumX and the regtest chain it is proxying.

It is not a must but can be a source of error.

I'm asking if we make electrum block and log to the terminal. I don't want it to run as a daemon.

CLI

./electrum-firo daemon -v --testnet

This is what you want. It is electrum daemon/rpc mode but does not fork the child process like the true daemon below.

Use -v for debug logging to stderr

True Daemon

./electrum-firo daemon --detach --testnet

	Default startup is CLI but starting with STARTUP="GUI"
	installs pyqt5 & starts the wallet Gui. Starting with
	STARTUP="DAEMON" forks the process.
@buck54321 buck54321 merged commit 3f8b0fe into decred:master Aug 31, 2023
@dev-warrior777 dev-warrior777 deleted the electrum_firo branch September 1, 2023 00:32
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.

5 participants