[electrum] split get_addr_io into get_address_unspent and get_address_spent
ClosedPublic
Actions

Authored by PiRK on Jul 11 2023, 11:11.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Commits

rABCb59a28904aac: [electrum] split get_addr_io into get_address_unspent and get_address_spent

Summary

The two operations were combined into a single function to factor the get_address_history call, but it turns out that in the slow cases that I encountered that call accounts for a minor part of the call time. The first operation is the real bottleneck in large single address wallets. For wallets that also produce a lot of outputs, both operations would probably be slow.

Some callsites only really need one of the two ouputs, most notably the add_input_info which has been found to be a source of GUI freezing.

Split the operations into their own functions that take the address history as a parameter, and keep the original function as a shortcut for when both outputs are actually needed.

Document and add typehints to all touched functions.

Remove an unused and poorly named get_addr_received function.

Test Plan

python test_runner.py

Test the application, use the send tab tosend a transaction or paste an address in the destination and an amount in the Amount tab which triggers fee estimation to exercise the changed codepaths.

Verify that get_addr_received is not used anywhere:
grep -r get_addr_received .

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Jul 11 2023, 11:11

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 11 2023, 11:11

PiRK requested review of this revision.Jul 11 2023, 11:11

Harbormaster completed remote builds in B24445: Diff 41374.Jul 11 2023, 11:12

PiRK added a child revision: D14249: [electrum] refactor make_unsigned_transaction to not generate the unspent coins every call.Jul 11 2023, 11:26

PiRK edited the summary of this revision. (Show Details)Jul 11 2023, 12:06

PiRK retitled this revision from [electrum] split get_addr_io into get_address_coins and get_address_spent to [electrum] split get_addr_io into get_address_unspent and get_address_spent.

Re: test plan

Test the application, use the send tab

could use a little more direction here. I'm reading this as "make sure it opens without crashing, and that clicking on the send tab also does not crash anything"

seems like this might not really be exhaustive enough. maybe I should load a specific wallet? Is it enough to just open it with any wallet, i.e. in this case we know every function touched here has been called?

electrum/electrumabc/wallet.py
926 ↗	(On Diff #41374)	used to be `for tx_hash, height in history` -- now `txid` instaed of `tx_hash` -- can this be called whatever or does it need to match something in the address_history list? I see type hints added to get_address_history. I can't really tell how `tx_hash` used to be correct but now its txid.
951 ↗	(On Diff #41374)	if this is unused then yes, should be removed. not clear though has removing it here is related to the diff. should be removed in its own diff. test plan for that diff should be about confirming that it's not used anywhere.

This revision now requires changes to proceed.Jul 11 2023, 18:14

PiRK edited the test plan for this revision. (Show Details)Jul 11 2023, 18:24

In D14248#318617, @bytesofman wrote:

Re: test plan

Test the application, use the send tab

I added some more specific instructions. Will update the commit shortly.

electrum/electrumabc/wallet.py
926 ↗	(On Diff #41374)	Here `txid` and `height` are just variables that are assigned in the context of the loop, so as long as they are consistent with what is used in the 3 lines just below, it is technically correct. I changed the variable name here because I want to migrate from the `transaction hash` terminology toward using `transaction id`, and this is a new function so I thought the whole block would just show up in the diff as new code, not as different code. I could revert and do the terminology change in its own diff.
951 ↗	(On Diff #41374)	OK. It is only loosely related to the diff as it calls one of the modified functions and if it weren't unused I would have change the code to use the new get_address_unspent function instead (as it does not use the `sent` var returned by `get_addr_io`)

bytesofman added inline comments.Jul 11 2023, 18:51

electrum/electrumabc/wallet.py
951 ↗	(On Diff #41374)	ok -- it's okay to keep if the test plan includes an easy way to verify that it's not used anywhere in the app

PiRK edited the test plan for this revision. (Show Details)Jul 11 2023, 18:59

use tx_hash, the naming convention is best changed in its own diff and for the whole file at once to avoid ambiguity about whether it really designates the same thing.

But keep using txid in new docstrings.

Harbormaster completed remote builds in B24454: Diff 41390.Jul 11 2023, 19:07

bytesofman requested changes to this revision.Jul 11 2023, 21:47

bytesofman added inline comments.

electrum/electrumabc/wallet.py
122 ↗	(On Diff #41390)	should the type definitions use tx_hash instead of txid now as well? if we plan to change everything from tx_hash to txid, should get that diff in soon
128 ↗	(On Diff #41390)	ditto
131 ↗	(On Diff #41390)	ditto

This revision now requires changes to proceed.Jul 11 2023, 21:47

PiRK added inline comments.Jul 12 2023, 05:11

electrum/electrumabc/wallet.py
122 ↗	(On Diff #41390)	The one thing that I'm not sure about is whether we want to also change in internal coin data structures (where it could break external plugins relying on "tx_hash") and in exported data (an old version of electrum no longer being able to read coin details files exported with a newer version, e.g. for proof generation) For now tx_hash and txid really are the same concepts, so changing it does not really improve anything. I just hope that the concepts will not become different in the future. Lotus and Bitcoin Core have gone that route of differentiating hash and id.