electrum icon indicating copy to clipboard operation
electrum copied to clipboard

`add_request` command used to reuse addresses of expired payment requests but not anymore

Open SomberNight opened this issue 3 years ago • 1 comments

The add_request command prior to version 4.3.0 used to reuse addresses that correspond to expired payment requests (but are otherwise unused). On current master however, it now returns False (unless --force is set, in which case it goes beyond the gap limit).

This is due to commit https://github.com/spesmilo/electrum/commit/bb2e4109422dc46652bc6579bcfc8149e3c8675

Relevant code for context: https://github.com/spesmilo/electrum/blob/ea5c49b3ab9cf0737192f1f4e637fd55c75e9b12/electrum/commands.py#L948-L962 https://github.com/spesmilo/electrum/blob/ea5c49b3ab9cf0737192f1f4e637fd55c75e9b12/electrum/wallet.py#L2254-L2260


I guess the change was made with the GUI in mind. For CLI/RPC, I think we should at least add an option to reuse expired addresses.

SomberNight avatar Aug 10 '22 22:08 SomberNight

Note: if we reuse an expired address and call wallet.create_request with that addr, we should also consider explicitly deleting the expired request from the wallet. Previously this was done implicitly as the keys for the two requests collided in the dict, but now that the key can be an RHASH, there could potentially be multiple requests in the dict with the same on-chain address.

Even unrelated to this issue, perhaps we might want to add sanity checks to forbid such collisions.

SomberNight avatar Aug 10 '22 22:08 SomberNight

indeed, we should be able to use addresses of expired requests. the issue with deleting them implicitly was that some user typed information was lost; we should not do that.

Perhaps we should not use the address as key in get_key_for_receive_request, when is_lightning is False That way, when a non-lightning request has expired, it would be possible to create a new request with the same address.

ecdsa avatar Aug 15 '22 11:08 ecdsa

Perhaps we should not use the address as key in get_key_for_receive_request, when is_lightning is False That way, when a non-lightning request has expired, it would be possible to create a new request with the same address.

Yes, maybe we should just have UUID (or similar) primary keys for both request and invoices, and have secondary maps to go from e.g. an address to a UUID. (so there would be no collisions in the primary uuid->invoice maps, but there might be collisions in the secondary maps, however we could enforce that if there is a collision, only one of those receive requests can be unpaid&unexpired) The general concept was also already suggested by @accumulator in https://github.com/spesmilo/electrum/issues/7777#issuecomment-1108263223

SomberNight avatar Aug 15 '22 12:08 SomberNight