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

Rotate taproot swap-in addresses #584

Merged
merged 9 commits into from
Feb 19, 2024
Merged

Rotate taproot swap-in addresses #584

merged 9 commits into from
Feb 19, 2024

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Jan 18, 2024

It uses groundwork made in branch swap-in-potentiam-taproot-rotate, but replaces hardcoded addresses by proper unbounded rotation.

Some cleanup is done at the end, by extracting wallets outside of Peer.

Best reviewed commit-by-commit.

@pm47 pm47 marked this pull request as draft January 18, 2024 15:44
@sstone sstone force-pushed the snapshot/swap-in-potentiam-taproot branch 3 times, most recently from 2f2b29c to d22dfd1 Compare January 24, 2024 15:23
@sstone sstone force-pushed the snapshot/swap-in-potentiam-taproot branch 2 times, most recently from 8fa7fea to c865b24 Compare January 29, 2024 20:36
@pm47 pm47 requested a review from sstone January 30, 2024 16:01
@sstone sstone force-pushed the snapshot/swap-in-potentiam-taproot branch from 91ea457 to 361faa9 Compare January 30, 2024 17:02
@sstone
Copy link
Member

sstone commented Jan 30, 2024

LGTM! There is yet no limit on the number of address that can be generated (it could be set explicitly in the wallet, or implicitly by the generator which could return null to signal that there are no more addresses)

@pm47 pm47 force-pushed the taproot-rotate-pm branch 2 times, most recently from 6387515 to 6eba0f8 Compare February 5, 2024 14:23
@sstone sstone force-pushed the snapshot/swap-in-potentiam-taproot branch from 02d89fb to 962efbb Compare February 6, 2024 13:21
@sstone sstone force-pushed the snapshot/swap-in-potentiam-taproot branch 3 times, most recently from ecbc4c6 to f963c21 Compare February 8, 2024 11:11
@sstone sstone force-pushed the snapshot/swap-in-potentiam-taproot branch 3 times, most recently from 5e18d0b to 7d17663 Compare February 13, 2024 14:00
Base automatically changed from snapshot/swap-in-potentiam-taproot to master February 15, 2024 09:22
@pm47
Copy link
Member Author

pm47 commented Feb 15, 2024

Rebased on master.

@pm47 pm47 requested a review from sstone February 15, 2024 10:06
@pm47 pm47 marked this pull request as ready for review February 15, 2024 10:06
Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit.
There is no cap on the number of addresses that can be generated. I don't think it's a problem: eletrum servers will add a 30 seconds delay when the "cost" of a connection becomes too expensive (only if a user has 100s of addresses).

@pm47
Copy link
Member Author

pm47 commented Feb 15, 2024

There is no cap on the number of addresses that can be generated. I don't think it's a problem: eletrum servers will add a 30 seconds delay when the "cost" of a connection becomes too expensive (only if a user has 100s of addresses).

I think that it's better than worrying about unintentional address reuse, and it will be regulated by increasingly slow sync times anyway.

pm47 and others added 9 commits February 15, 2024 18:06
This meta notably contains the index if the address was
deterministically generated.
We create and monitor a fixed number of different swap-in addresses.
Peer will choose an empty swap-in addresses if there is one, or a random address if there are none.
We limit the number of monitored swap-in addresses to avoid hitting resource limits on Electrum servers.
A few hundred addresses (100 or 200) is a safe limit.
We keep the core logic for watching individual addresses, and just
generate more when needed.
It's not really synchronous, but we rely the least possible on the
mailbox. This makes the `window` disappear because we are scanning
addresses one by one.

Suggested by @sstone.
It's more consistent with Utxo and makes jvm interop easier.
@pm47 pm47 merged commit 506003f into master Feb 19, 2024
2 checks passed
@pm47 pm47 deleted the taproot-rotate-pm branch February 19, 2024 10:14
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.

2 participants