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

Reserved address pool #531

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Reserved address pool #531

merged 1 commit into from
Oct 30, 2024

Conversation

dangeross
Copy link
Collaborator

@dangeross dangeross commented Oct 16, 2024

This PR:

  • Adds addresses allocated to swaps to a reserved pool of addresses until the swap expires
  • Gets the next unused address from either an expired reserved address or the next index from the cached derivation index
  • Caches the last used derivation index
  • Associates a magic routing hint address to the reverse swap, which then when paid via this address, is linked to the reverse swap (if the address is unused, it will be reallocated after expiry)

TODO:

  • Adjust the payer_amount_sat and receiver_amount_sat if paid via magic routing hint to set the payment fees to 0 (both should equal tx amount)
  • Patch LWK to allow an "up to index" full scan to prevent gap limit errors

Fixes #523

@dangeross dangeross linked an issue Oct 16, 2024 that may be closed by this pull request
@dangeross dangeross marked this pull request as ready for review October 17, 2024 14:29
@ok300
Copy link
Contributor

ok300 commented Oct 18, 2024

Adds addresses allocated to swaps to a reserved pool of addresses until the swap expires

Associates a magic routing hint address to the reverse swap, which then when paid via this address, is linked to the reverse swap

What happens when a Receive Swap is paid via the MRH Liquid address after the swap expired? Will this also be associated with the swap?

Also what happens multiple payments are made to the MRH Liquid address, i.e. address re-use? Will those payments be associated with the same swap?

@roeierez
Copy link
Member

What happens when a Receive Swap is paid via the MRH Liquid address after the swap expired? Will this also be associated with the swap?

Since the magic hints is an internal mechanism hidden inside the invoice clients should parse the invoice in order to pay. The expected behavior by clients is to return error (invoice expired) immediately and not continue to pay so there should not be payments after the swap has expired. If clients other than the sdk do not behave this way then yes worst case we might have multiple payments associated. I think associating the first one only is reasonable.

@dangeross
Copy link
Collaborator Author

Also what happens multiple payments are made to the MRH Liquid address, i.e. address re-use? Will those payments be associated with the same swap?

The first one is associated while the swap is ongoing, then any following ones are stored as separate payments

@dangeross
Copy link
Collaborator Author

What happens when a Receive Swap is paid via the MRH Liquid address after the swap expired? Will this also be associated with the swap?

Like Roei said, the first step should be to check if the invoice has expired, then extract the MRH (I'm not sure if Boltz returns the BIP21 address if the swap is expired, need to check this). But to answer the question, the it won't be associated as the swap is no longer ongoing and therefore will be a separate payment.

lib/core/src/sdk.rs Outdated Show resolved Hide resolved
lib/core/src/sdk.rs Outdated Show resolved Hide resolved
@dangeross dangeross force-pushed the savage-reserved-addresses branch 2 times, most recently from a7a3a7d to e0d0d2e Compare October 18, 2024 10:29
@dangeross dangeross changed the base branch from signer to main October 20, 2024 07:00
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looking good.

lib/core/src/persist/receive.rs Show resolved Hide resolved
lib/core/src/wallet.rs Outdated Show resolved Hide resolved
lib/core/src/wallet.rs Outdated Show resolved Hide resolved
lib/core/src/persist/cache.rs Show resolved Hide resolved
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Almost good to go, one comment.

lib/core/src/wallet.rs Outdated Show resolved Hide resolved
lib/core/src/model.rs Show resolved Hide resolved

fn delete_reserved_address_inner(tx: &Transaction, address: &str) -> Result<(), PaymentError> {
tx.execute(
"DELETE FROM reserved_addresses WHERE address = ?",
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Not sure what's the best way to delete data in the context of real-type sync.

IMO DELETE statements could lead to edge-cases when 2 SDK instances sync, one has the data (here, the reserved_address row) and the other doesn't. Depending on the ordering and timing, it could be that one or the other is treated as "the truth".

In contrast, sync would be more resilient if we delete by setting a flag, i.e. reserved = false. Any such conflict during synchronization could then be resolved simply: if any of the two have reserved = false, then its false.

@roeierez @hydra-yse what do you think, does this matter? Or it makes no difference for real-time sync?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reserved address table currently only stores MRH addresses used in receive swaps. These are the two cases where a delete would occur:

  • A pay to the MRH address is seen while there is no swap progress.
  • The reserved address block height expires and the address is reused for another purpose (claim, refund, MRH, direct address). In the MRH case it is re-added to the reserved address table with a later expiry.

Also to note is that the last_derivation_index from the cache needs to be synced.

Maybe it's best to add this as a task of the real-time sync tracking issue

lib/core/src/persist/mod.rs Show resolved Hide resolved
lib/core/src/persist/receive.rs Show resolved Hide resolved
@dangeross dangeross requested a review from ok300 October 24, 2024 13:06
Copy link
Contributor

@ok300 ok300 left a comment

Choose a reason for hiding this comment

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

Looks good.

The only remaining NIT is the question related to real-time sync, but IMO it can be handled separaately if necessary.

@dangeross dangeross force-pushed the savage-reserved-addresses branch 2 times, most recently from fe107ab to 38f81a7 Compare October 29, 2024 14:33
@dangeross dangeross changed the base branch from main to savage-prevent-double-claim October 29, 2024 14:34
Base automatically changed from savage-prevent-double-claim to main October 30, 2024 09:08
@dangeross dangeross merged commit 4d036f2 into main Oct 30, 2024
8 checks passed
@dangeross dangeross deleted the savage-reserved-addresses branch October 30, 2024 10:06
roeierez added a commit that referenced this pull request Nov 5, 2024
* commit 'a515718dc72c1c8eb62cecb7ad9c9862e8dc727c':
  feat: allow send transition from `TimedOut` to `Created` (#545)
  Propagate signer interface changes (#544)
  fix: double-lockup when payment is TimedOut (#541)
  Reserved address pool (#531)
  Prevent swap double claim (#542)
  Receive Chain Swap: support refund even when lockup address is re-used (#471)
  Bring remaining u32 amount types to u64 (#537)
  Add swift framework plists (#536)
  Update README.md
  Update README.md
  Update roadmap
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.

Missing event when bolt11 is paid via magic hints.
3 participants