Refactor and cleanup of blockchaininterface and related #1462

kristapsk · 2023-03-17T02:16:38Z

Summary of changes:

Add typehints to jmclient/jmclient/blockchaininterface.py and jmclient/jmclient/jsonrpc.py;
Move all methods called by external code to BlockchainInterface base class or add abstract methods there;
~~Remove broken ElectrumWalletInterface (we can re-introduce it later from history if somebody wants to fix it);~~ (done in Remove ElectrumWalletInterface #1477)
More dummy abstract method overrides for DummyBlockchainInterface (for tests);
Alphabetical ordering of imports and other minor stuff;
Behaviour change - previously fee estimation would fail if it could not get mempoolminfee, now will go with default 10 sat/vB.

#1460 was part of this, but did separate PR for that one, as it is simpler to review small refactoring changes.

This should make it more easy to: 1) write new code that uses blockchaininterface, 2) write new alternative implementations of BlockchainInterface.

kristapsk · 2023-03-17T02:18:33Z

So far have tested locally that tests are passing. Next is to do some more manual testing with signet / testnet. But anyway would be good to have some feedback.

jmclient/jmclient/configure.py

jmclient/jmclient/jsonrpc.py

jmclient/jmclient/payjoin.py

jmclient/jmclient/blockchaininterface.py

AdamISZ · 2023-03-21T22:29:54Z

Completed a first round of reading review. Apart from the issues raised, otherwise it seems good to me.

I think #1461 should be tested and merged first, since there is a demand from users for that. (Mentioning it because they will conflict).

Related point, since this change is very big and invasive, we really need to test it a lot with all the various usage modes, before we merge it.

kristapsk · 2023-03-21T22:54:46Z

@AdamISZ Thanks, will look at reviewing and testing #1461 soon.

kristapsk · 2023-03-22T15:33:25Z

Related point, since this change is very big and invasive, we really need to test it a lot with all the various usage modes, before we merge it.

Switched my public orderbook mirror (http://nnuifroxn5aolsqa2svedcskojlqfp2ygt4u42ac7njehsbemagpwiqd.onion/) to this code.

PulpCattel

Just skimmed through, will try to look more into this.
Since we are touching, we could reformat the entire file with some linter (I still see some PEP8 complaints). Anyway, nbd.

jmclient/jmclient/jsonrpc.py

jmclient/jmclient/blockchaininterface.py

PulpCattel · 2023-03-23T12:39:57Z

jmclient/jmclient/blockchaininterface.py

@@ -689,7 +692,7 @@ def listunspent(self):
            "amount": u["amount"]
        } for u in self.scan_result["unspents"]]

-    def set_wallet_no_history(self, wallet):
+    def set_wallet_no_history(self, wallet) -> None:


We could add typehint for wallet.

Tried to do that, but ran into problem and don't want to spend too much time just on a single typehint.

It should be BaseWallet, which requires moving INF_HEIGHT out from blockchaininterface, so that there is no circular imports and we can from jmclient.wallet import BaseWallet, ended up with some strange errors in tests, couldn't figure out:

self = <test_payjoin.TrialTestPayjoin1 testMethod=test_payment> def tearDown(self): for dc in reactor.getDelayedCalls(): dc.cancel() res = final_checks(self.wallet_services, self.cj_amount, > self.manager.final_psbt.get_fee(), self.ssb, self.rsb) E AttributeError: 'TrialTestPayjoin1' object has no attribute 'manager' jmclient/test/test_payjoin.py:133: AttributeError

And flake8 doesn't like forward references, which could be an alternative solution.

Agreed not to waste too much time. If you want to try one last time, it seems to work for me locally using:

from __future__ import annotations # at the very top # <-- Additional import. import typing if typing.TYPE_CHECKING: from jmclient.wallet import BaseWallet

kristapsk · 2023-03-28T00:59:19Z

I could try splitting off some parts of this PR into smaller PRs, so that it's easier to review and test. For example, fee estimation code changes.

AdamISZ · 2023-04-11T21:32:49Z

I could try splitting off some parts of this PR into smaller PRs, so that it's easier to review and test. For example, fee estimation code changes.

This seems to make a lot of sense. One file or group of files at a time.

3805c7a Remove ElectrumWalletInterface (Kristaps Kaupe) Pull request description: Remove broken `ElectrumWalletInterface` (we can re-introduce it later from history if somebody wants to fix it). Part of splitting #1462 into smaller PRs for easier testing and reviews. ACKs for top commit: AdamISZ: tACK 3805c7a Tree-SHA512: 8d11f3488c5e16058be187212b49e2cf11787b5fb9a1bb87f8949f76fa1569358932bea5bcf4fa3d0b0640534d87681d762b6e595f85a8e02584c9e438960c66

kristapsk · 2023-04-21T15:33:50Z

Another failure of abstraction that needs to be addressed -

joinmarket-clientserver/jmclient/jmclient/wallet_utils.py

Lines 506 to 510 in 71c0a84

    
           #TODO it is a failure of abstraction here that 
        
           # the bitcoin core interface is used directly 
        
           #the function should either be removed or added to bci 
        
           #or possibly add some kind of `gettransaction` function 
        
           # to bci

.

…etwalletinfo()` for `bci` e31e839 Add get_wallet_rescan_status() instead of getwalletinfo() for bci (Kristaps Kaupe) Pull request description: Noticed this when tried to rebase #1462 after merging of #1477. #1461 added public `getwalletinfo()` method to `BitcoinCoreInterface`, which was used by code outside of `jmclient/jmclient/blockchaininterface.py`. This is bad approach, as it relies on Bitcoin Core RPC `getwalletinfo` returned `dict`, which contains a lots of different stuff too, could lead to more problems in future introducing other blockchain interface classes. Let's instead have generic method returning just wallet rescan status. Also it now returns `Tuple[bool, Optional[Decimal]]` with rescan status percentage, if rescan is in progress, although that's not used by any other code for now. ACKs for top commit: AdamISZ: utACK e31e839 , very much agree with the thinking here. Tree-SHA512: 2d8c9b8157847e713838099d0f62dfcd5321c9498cf8453a9087407e2cd9c32906739c8e71460fc6ac6426662d2ac501261080fea08388d928933f788bda9a8d

kristapsk · 2023-04-30T19:43:33Z

Rebased

kristapsk · 2023-05-09T17:30:18Z

Rebased against master to re-run CI with latest fixes.

kristapsk · 2023-06-06T14:18:28Z

Rebased

d5c240b Refactor fee estimation code (Kristaps Kaupe) Pull request description: Part of splitting #1462 into smaller PRs for easier reviewing and testing. Top commit has no ACKs. Tree-SHA512: af1d75881ac7736c46ae8ac96a50808558b626c92f52ee290e68eb19d2e317d0e46c553f7087ed609788611c00de3c3783bf8246e7286da63673212a7f102b25

kristapsk · 2023-07-27T12:55:22Z

Rebased

kristapsk · 2023-08-01T08:50:23Z

I think I have addressed or responded to all review comments.

AdamISZ · 2023-08-01T18:10:25Z

Testing on 16a221a ; first, current master, then, that commit (i.e. this PR):

(jmvenv)a@jmtest-1:~/joinmarket-clientserver$ pytest -v --btcconf=/home/a/bitcoin.conf --btcroot=/home/a/bitcoin-24.1/bin/ --btcpwd=123456abcdef --nirc=2 -p no:warnings -k test_payjoin
================================================================================== test session starts ==================================================================================
platform linux -- Python 3.9.2, pytest-6.2.5, py-1.11.0, pluggy-1.2.0 -- /home/a/joinmarket-clientserver/jmvenv/bin/python3
cachedir: .pytest_cache
rootdir: /home/a/joinmarket-clientserver, configfile: setup.cfg, testpaths: jmbitcoin, jmclient, jmbase, jmdaemon, test
plugins: cov-2.5.1
collected 403 items / 398 deselected / 5 selected                                                                                                                                       

jmclient/test/test_payjoin.py::TrialTestPayjoin1::test_payment PASSED                                                                                                             [ 20%]
jmclient/test/test_payjoin.py::TrialTestPayjoin2::test_bech32_payment PASSED                                                                                                      [ 40%]
jmclient/test/test_payjoin.py::TrialTestPayjoin3::test_multi_input PASSED                                                                                                         [ 60%]
jmclient/test/test_payjoin.py::TrialTestPayjoin4::test_low_feerate PASSED                                                                                                         [ 80%]
jmclient/test/test_psbt_wallet.py::test_payjoin_workflow[0.05-SegwitLegacyWallet-SegwitLegacyWallet] PASSED                                                                       [100%]

========================================================================== 5 passed, 398 deselected in 17.48s ===========================================================================
(jmvenv)a@jmtest-1:~/joinmarket-clientserver$ git checkout pr_1462
Switched to branch 'pr_1462'
(jmvenv) a@jmtest-1:~/joinmarket-clientserver$ rm -rf ~/.bitcoin/regtest
(jmvenv) a@jmtest-1:~/joinmarket-clientserver$ pytest -v --btcconf=/home/a/bitcoin.conf --btcroot=/home/a/bitcoin-24.1/bin/ --btcpwd=123456abcdef --nirc=2 -p no:warnings -k test_payjoin
================================================================================== test session starts ==================================================================================
platform linux -- Python 3.9.2, pytest-6.2.5, py-1.11.0, pluggy-1.2.0 -- /home/a/joinmarket-clientserver/jmvenv/bin/python3
cachedir: .pytest_cache
rootdir: /home/a/joinmarket-clientserver, configfile: setup.cfg, testpaths: jmbitcoin, jmclient, jmbase, jmdaemon, test
plugins: cov-2.5.1
collected 403 items / 398 deselected / 5 selected                                                                                                                                       

jmclient/test/test_payjoin.py::TrialTestPayjoin1::test_payment PASSED                                                                                                             [ 20%]
jmclient/test/test_payjoin.py::TrialTestPayjoin2::test_bech32_payment PASSED                                                                                                      [ 40%]
jmclient/test/test_payjoin.py::TrialTestPayjoin3::test_multi_input FAILED                                                                                                         [ 60%]
jmclient/test/test_payjoin.py::TrialTestPayjoin3::test_multi_input ERROR                                                                                                          [ 60%]
jmclient/test/test_payjoin.py::TrialTestPayjoin4::test_low_feerate PASSED                                                                                                         [ 80%]
jmclient/test/test_psbt_wallet.py::test_payjoin_workflow[0.05-SegwitLegacyWallet-SegwitLegacyWallet] FAILED                                                                       [100%]

Thus these are errors introduced by the PR. The errors all appear to be fee estimation related:

RuntimeError: Cannot estimate fee per kB, possibly a failure of connection to the blockchain.

... these errors don't only appear in the test_payjoin section, they crop up in a few other places.

kristapsk · 2023-08-01T18:20:19Z

Will look at this! Apparently, some commit I added today causes this.

kristapsk · 2023-08-01T20:08:49Z

Ok, I'm giving up for today, will look at it again later. I cannot understand how this error can happen as after #1504 changes estimate_fee_per_kb() never returns None...

        if jm_single().bc_interface is None:
            raise RuntimeError("Cannot estimate transaction fee " +
                "without blockchain source.")
        fee_per_kb = jm_single().bc_interface.estimate_fee_per_kb(
                    jm_single().config.getint("POLICY","tx_fees"))
        if fee_per_kb is None:
>           raise RuntimeError("Cannot estimate fee per kB, possibly" +
                               " a failure of connection to the blockchain.")
E           RuntimeError: Cannot estimate fee per kB, possibly a failure of connection to the blockchain.

/home/user/joinmarket-clientserver/jmvenv/lib/python3.9/site-packages/jmclient/wallet.py:83: RuntimeError

AdamISZ · 2023-08-01T21:18:12Z

So far this is proving to be quite tricky.

On this PR, 16a221a:

run pytest only on the module test_psbt_wallet. It fails at line 280 of test_psbt_wallet in the test function test_payjoin_workflow, in the call to estimate_tx_fee. If you, however, run only that one test function, it passes.
This pattern seems consistent.

On master, 54db582 :
It always passes.

kristapsk · 2023-08-01T21:26:28Z

It also fails now with dd86234, where previously there were no CI errors...

AdamISZ · 2023-08-02T16:03:31Z

The problem is here:

joinmarket-clientserver/jmclient/jmclient/blockchaininterface.py

Lines 292 to 298 in 16a221a

    
           if retval < mempoolminfee_in_sat: 
        
               msg = "Using this mempool min fee as tx feerate" 
        
               if tx_fees_factor != 0: 
        
                   msg = msg + " (randomized for privacy)" 
        
               log.info(msg + ": " + btc.fee_per_kb_to_str( 
        
                   mempoolminfee_in_sat_randomized) + ".") 
        
               return int(mempoolminfee_in_sat_randomized)

in master, there is a following else clause:

joinmarket-clientserver/jmclient/jmclient/blockchaininterface.py

Lines 135 to 148 in d5c240b

    
           if retval < mempoolminfee_in_sat: 
        
               msg = "Using this mempool min fee as tx feerate" 
        
               if tx_fees_factor != 0: 
        
                   msg = msg + " (randomized for privacy)" 
        
               log.info(msg + ": " + btc.fee_per_kb_to_str( 
        
                   mempoolminfee_in_sat_randomized) + ".") 
        
               return int(mempoolminfee_in_sat_randomized) 
        
           else: 
        
               msg = "Using bitcoin network feerate for " + str(tx_fees) + \ 
        
                   " block confirmation target" 
        
               if tx_fees_factor != 0: 
        
                   msg = msg + " (randomized for privacy)" 
        
               log.info(msg + ": " + btc.fee_per_kb_to_str(retval)) 
        
               return int(retval)

Not sure why it's missing, but without it we just return None if we're above the mempool min.

kristapsk · 2023-08-02T16:05:09Z

The problem is here:

joinmarket-clientserver/jmclient/jmclient/blockchaininterface.py

Lines 292 to 298 in 16a221a

if retval < mempoolminfee_in_sat:

msg = "Using this mempool min fee as tx feerate"

if tx_fees_factor != 0:

msg = msg + " (randomized for privacy)"

log.info(msg + ": " + btc.fee_per_kb_to_str(

mempoolminfee_in_sat_randomized) + ".")

return int(mempoolminfee_in_sat_randomized)

in master, there is a following else clause:

joinmarket-clientserver/jmclient/jmclient/blockchaininterface.py

Lines 135 to 148 in d5c240b

if retval < mempoolminfee_in_sat:

msg = "Using this mempool min fee as tx feerate"

if tx_fees_factor != 0:

msg = msg + " (randomized for privacy)"

log.info(msg + ": " + btc.fee_per_kb_to_str(

mempoolminfee_in_sat_randomized) + ".")

return int(mempoolminfee_in_sat_randomized)

else:

msg = "Using bitcoin network feerate for " + str(tx_fees) + \

" block confirmation target"

if tx_fees_factor != 0:

msg = msg + " (randomized for privacy)"

log.info(msg + ": " + btc.fee_per_kb_to_str(retval))

return int(retval)

Not sure why it's missing, but without it we just return None if we're above the mempool min.

Thanks! Failure to rebase from my side, master version is correct, as this was changed in #1504, which was taken from here.

AdamISZ · 2023-08-02T19:08:37Z

tACK 92060eb

Tested: manual spends, individual coinjoins, and tumbler operation. Also ran test suite ofc. Not currently seeing any problems, though, no objection if you still need to clean some details up. Also reminder to squash before merge. Thanks.

kristapsk · 2023-08-02T20:53:39Z

@PulpCattel Care to take another look at this?

PulpCattel · 2023-08-03T08:33:02Z

Looking at it now.

PulpCattel

Tests pass, but I didn't test much manually other than basic send.

jmclient/jmclient/blockchaininterface.py

PulpCattel · 2023-08-03T07:36:37Z

jmclient/jmclient/blockchaininterface.py

        return len(self._rpc('getaddressinfo', [addr])['labels']) > 0

-    def get_block(self, blockheight):
+    def get_block(self, blockheight: int) -> Optional[str]:


Question in dd86234:

Is Optional correct here? I see that a few methods (e.g., this one or get_transaction()) use Optional, but many others don't.

Don't see any reason for Optional here. The function misses error checking (what if get block hash fails), but that's orthogonal to typehints, also, this has only ever been used for blockchain scanning so far.

I suppose there's an argument for Optional based on an idea of None being used as the return type for failure? But that's just imagined, not actual. I'd just have str only, since that's the function's intent, and exception classes can handle failures.

Agree, addressed in 094df20.

PulpCattel · 2023-08-03T07:51:17Z

jmclient/jmclient/blockchaininterface.py

+    def import_addresses(self, addr_list: List[str], wallet_name: str,
+                         restart_cb: Callable[[str], None] = None) -> None:


In dd86234:

addr_list can receive a set. For example import_addresses_if_needed() below would pass a set to it. But I see that in other places is called with a list.

IDK how convenient it is to use set everywhere, or we could change this to Iterable[str] or Union[List[str], Set[str]].

Iterable seems nicer.

Addressed in 52b007e.

PulpCattel · 2023-08-03T08:05:39Z

jmclient/jmclient/blockchaininterface.py

+    def get_current_block_height(self) -> int:
        try:
-            res = self._rpc("getblockcount", [])
+            return self._rpc("getblockcount", [])
        except JsonRpcError as e:
-            log.error("Getblockcount RPC failed with: %i, %s" % (
+            raise RuntimeError("Getblockcount RPC failed with: %i, %s" % (
                e.code, e.message))


General note/unrelated to this PR:

It is a bit weird that this is the only method checking for JsonRpcError.
I see the check was added in a2aafd2 with the idea of eventually improving this. Maybe we could leave a TODO

Should be addressed separately IMO.

PulpCattel · 2023-08-03T08:27:55Z

jmclient/jmclient/blockchaininterface.py

@@ -689,7 +692,7 @@ def listunspent(self):
            "amount": u["amount"]
        } for u in self.scan_result["unspents"]]

-    def set_wallet_no_history(self, wallet):
+    def set_wallet_no_history(self, wallet) -> None:


Agreed not to waste too much time. If you want to try one last time, it seems to work for me locally using:

from __future__ import annotations # at the very top # <-- Additional import. import typing if typing.TYPE_CHECKING: from jmclient.wallet import BaseWallet

PulpCattel · 2023-08-03T08:33:57Z

jmclient/jmclient/jsonrpc.py

+        Send an appropriate HTTP query to the server.  The JSON-RPC
+        request should be (as object) in 'obj'.  If the call succeeds,
+        the resulting JSON object is returned.  In case of an error
+        with the connection (not JSON-RPC itself), an exception is raised.


Nit in dd86234:

Extra spaces

Suggested change

Send an appropriate HTTP query to the server. The JSON-RPC

request should be (as object) in 'obj'. If the call succeeds,

the resulting JSON object is returned. In case of an error

with the connection (not JSON-RPC itself), an exception is raised.

Send an appropriate HTTP query to the server. The JSON-RPC

request should be (as object) in 'obj'. If the call succeeds,

the resulting JSON object is returned. In case of an error

with the connection (not JSON-RPC itself), an exception is raised.

Addressed in 60b8923.

PulpCattel · 2023-08-03T08:44:20Z

jmclient/jmclient/wallet_service.py

@@ -964,7 +964,7 @@ def get_internal_addr(self, mixdepth):
        self.import_addr(addr)
        return addr

-    def collect_addresses_init(self):
+    def collect_addresses_init(self) -> Set[str]:


In c51f349:

This method returns return addresses, saved_indices, my IDE says it's Tuple[Set[str], Dict[int, List[int]]]

Suggested change

def collect_addresses_init(self) -> Set[str]:

def collect_addresses_init(self) -> Tuple[Set[str], Dict[int, List[int]]]:

Addressed in d80d8b6.

kristapsk · 2023-08-22T07:57:26Z

Addressed review comments.

kristapsk · 2023-09-08T08:18:37Z

@AdamISZ @PulpCattel Any more comments? I would like to merge this soon.

AdamISZ · 2023-09-08T17:45:41Z

@AdamISZ @PulpCattel Any more comments? I would like to merge this soon.

Taking a look now, just to sanity check everything.

AdamISZ · 2023-09-08T18:53:30Z

tACK d80d8b6

Squash before merging reminder :)

Summary of changes: * Add typehints to blockchaininterface.py and jsonrpc.py; * Move all methods called by external code to BlockchainInterface base class or add abstract methods there; * More dummy abstract method overrides for DummyBlockchainInterface (for tests); * Alphabetical ordering of imports and other minor stuff; * Behaviour change - previously fee estimation would fail if it could not get mempoolminfee, now will go with default 10 sat/vB. Co-authored-by: Pulp <[email protected]>