Page MenuHomePhabricator

[electrum] remove server.relay_fee API call
ClosedPublic

Authored by PiRK on Mon, Jul 1, 18:54.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC35c39faff542: [electrum] remove server.relay_fee API call
Summary

The relay_fee is unused, so no need to ask the server about it.

Test Plan

Run the Application for a few minutes, connect to another fulcrum server, wait a few minutes, make sure basic features still work

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Mon, Jul 1, 18:54

I can imagine a use case for the banner, which is checking you're connecting the expected server. Any strong reason to remove this, which seems harmless ?

I can imagine a use case for the banner, which is checking you're connecting the expected server. Any strong reason to remove this, which seems harmless ?

No strong reason, just that it is not very much used at the moment, and it would be one less thing to implement in chronik. And if anyone can fake the url, it is even easier to fake the banner.
If you think it can be useful, I can remove it from that diff and make the data more accessible in a place that makes more sense (in the network menu for instance)

Fabien requested changes to this revision.Mon, Jul 1, 21:14
In D16393#371123, @PiRK wrote:

I can imagine a use case for the banner, which is checking you're connecting the expected server. Any strong reason to remove this, which seems harmless ?

No strong reason, just that it is not very much used at the moment, and it would be one less thing to implement in chronik. And if anyone can fake the url, it is even easier to fake the banner.
If you think it can be useful, I can remove it from that diff and make the data more accessible in a place that makes more sense (in the network menu for instance)

Really that's trivial to add to the electrum facade, even if it's just returning the version number or something like that. I would just keep it for now.

This revision now requires changes to proceed.Mon, Jul 1, 21:14
PiRK retitled this revision from [electrum] remove server.banner and server.relay_fee API calls to [electrum] remove server.relay_fee API call.
PiRK edited the summary of this revision. (Show Details)
PiRK edited the test plan for this revision. (Show Details)

only remove 'relay_fee', keep 'banner'

Tail of the build log:

-- Found BerkeleyDB component CXX: /usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so
-- Found BerkeleyDB: /usr/include (found suitable version "5.3.28", minimum required is "5.3") found components: CXX 
-- Found SQLite3: /usr/include (found suitable version "3.34.1", minimum required is "3.7.17") 
-- Found ZeroMQ component zmq: /usr/lib/x86_64-linux-gnu/libzmq.so
-- Found ZeroMQ: /usr/include (found suitable version "4.3.4", minimum required is "4.1.5")  
-- Could NOT find Protobuf (missing: Protobuf_DIR)
-- Found Protobuf: /usr/lib/x86_64-linux-gnu/libprotobuf.so;-pthread (found version "3.12.4") 
-- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "1.1.1w")  
-- Looking for EVP_MD_CTX_new
-- Looking for EVP_MD_CTX_new - found
-- Found QREncode component qrencode: /usr/lib/x86_64-linux-gnu/libqrencode.so
-- Found QREncode: /usr/include   
-- Configuring native build in /work/abc-ci-builds/electrum-tests/native
-- Configuring done
-- Generating done
-- Build files have been written to: /work/abc-ci-builds/electrum-tests
[1/2] link test_runner.py
[2/2] Run Electrum ABC unit tests...
FAILED: electrum/CMakeFiles/check-electrum 
cd /work/abc-ci-builds/electrum-tests/electrum && /usr/bin/python3.9 ./test_runner.py
[secp256k1] warning: libsecp256k1 library failed to load
[secp256k1] warning: loading from /work/electrum/electrumabc/libsecp256k1.so.0 failed with: /work/electrum/electrumabc/libsecp256k1.so.0: cannot open shared object file: No such file or directory
[secp256k1] warning: loading from libsecp256k1.so.0 failed with: libsecp256k1.so.0: cannot open shared object file: No such file or directory
Testing `setup.py --version`: OK

[ecc] info: libsecp256k1 library not available, falling back to python-ecdsa. This means signing operations will be slower. Try running:

  $  contrib/make_secp

(You need to be running from the git sources for contrib/make_secp to be available)
Traceback (most recent call last):
  File "/work/electrum/electrumabc_plugins/trezor/trezor.py", line 28, in <module>
    from trezorlib.messages import (
ImportError: cannot import name 'RecoveryDeviceType' from 'trezorlib.messages' (/usr/local/lib/python3.9/dist-packages/trezorlib/messages.py)
...........................................................................................................................................127.0.0.1 - - [02/Jul/2024 14:32:54] "GET /invoice HTTP/1.1" 503 -
.127.0.0.1 - - [02/Jul/2024 14:32:54] "GET / HTTP/1.1" 200 -
.127.0.0.1 - - [02/Jul/2024 14:32:54] "GET /invoice HTTP/1.1" 200 -
127.0.0.1 - - [02/Jul/2024 14:32:54] "POST /pay HTTP/1.1" 200 -
.127.0.0.1 - - [02/Jul/2024 14:32:54] "GET / HTTP/1.1" 200 -
...s........................[trezor] Library version for 'trezor' is incompatible.
Installed: 0.13.9, Needed: 0.13.8 <= x < 0.14
...................................................................................................................s.........sss.............................s.s...[dummy name] Library version for 'dummy name' is incompatible.
Installed: 0.13.9, Needed: 0.13.8 <= x < 0.14
E
======================================================================
ERROR: test_electrum_tx_to_txtype (electrumabc_plugins.trezor.tests.test_trezor.TestTrezorPlugin)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc_plugins/trezor/tests/test_trezor.py", line 53, in test_electrum_tx_to_txtype
    self.assertEqual(plugin.electrum_tx_to_txtype(tx, xpub_path), expected_txtype)
  File "/work/electrum/electrumabc_plugins/trezor/trezor.py", line 597, in electrum_tx_to_txtype
    t = TransactionType()
NameError: name 'TransactionType' is not defined

----------------------------------------------------------------------
Ran 330 tests in 23.445s

FAILED (errors=1, skipped=7)
ninja: build stopped: cannot make progress due to previous errors.
Build electrum-tests failed with exit code 1
Fabien requested changes to this revision.Tue, Jul 2, 19:02

This needs a rebase to fix the build failure

This revision now requires changes to proceed.Tue, Jul 2, 19:02
This revision is now accepted and ready to land.Wed, Jul 3, 05:22
This revision was automatically updated to reflect the committed changes.