Implement auto-cycling of Noble deposit addresses #4878
Conversation
zbuc
force-pushed
the
4873_noble_rotation
branch
from
c78757c
to
5e8ea2b
Compare
cronokirby
added
the
pr-needs-testing
| // This means the midpoint had a deposit in it waiting for registration. | ||
| // This will "flush" this unregistered address, however the user still wants a new one, so return the midpoint + 1. | ||
| Ok(mid + 1) |
There was a problem hiding this comment.
What about these cases:
- Both sequence 23 and sequence 24 has funds in it. Binary search ends up hitting 23 first with a registration call. It succeeds in flushing the funds and then this function returns 24. We display that address to the user, but when we poll, we successfully flush that one too. Then the user attempts to deposit into that address. Will that deposit bounce as it's already registered?
- What if we flush the max sequence number. Would this return max + 1?
There was a problem hiding this comment.
-
The program should check the address before it is displayed to ensure it's fresh (could have been used by another instance on another machine)
-
Yes it should wrap around after 2^16 deposits
There was a problem hiding this comment.
Re point 2:
With a binary search, how there could be a notion of wrapping around? All sequence numbers will have been exhausted. Or are you suggesting once all are registered, we just offer up sequence 0 to re-use? Edit: or just pick one at random?
There was a problem hiding this comment.
Wrap-around is hard to do without state tracking, so I think using a random sequence number (and potentially notifying the user that it's being re-used) is a good idea
| if left == mid || right == mid { | ||
| // We've iterated as far as we can, the next sequence number | ||
| // after the midpoint should be the next available sequence number. | ||
| return Ok(mid + 1); | ||
| } |
There was a problem hiding this comment.
What if all sequence numbers have been registered? Will this return max + 1?
| match code { | ||
| 9 => Ok(NobleRegistrationResponse::NeedsDeposit), | ||
| 0 => Ok(NobleRegistrationResponse::Success), | ||
| 19 => Ok(NobleRegistrationResponse::AlreadyRegistered), | ||
| _ => Err(anyhow::anyhow!("unknown response from Noble")), |
There was a problem hiding this comment.
On web, we don't get the code 19, but below when submitting a tx with the same sequence fyi. Looks like a cometbft mempool err.
{"code":-32603,"message":"Internal error","data":"tx already exists in cache"}
| async fn get_next_noble_sequence( | ||
| account: Option<u32>, | ||
| fvk: &FullViewingKey, | ||
| channel: &str, | ||
| noble_node: &Url, | ||
| ) -> Result<u16> { | ||
| // perform binary search to find the first unused noble sequence number | ||
| // search space (sequence number) is 2 bytes wide | ||
| let left = 0u16; | ||
| let right = 0xffffu16; | ||
| let mid = (left + right) / 2u16; | ||
|
|
||
| // attempt to register midpoint | ||
| _get_next_noble_sequence(left, right, mid, noble_node, channel, fvk, account).await | ||
| } |
There was a problem hiding this comment.
Here is a web implementation of binary search, inspired by this, but handles edge cases a bit differently: https://github.com/prax-wallet/prax/pull/215/files#diff-c569e62334fe6b19c6a1434837ca111daa9f4eb8287e7ab0c4925564c2ff44c2
zbuc
force-pushed
the
4873_noble_rotation
branch
from
5e8ea2b
to
46ac8d0
Compare
|
The Noble testnet has changed since this PR was first authored. We believe the encoding error has been resolved, but there's a temporary blocker in that the current Noble testnet is charging gas fees for registration transactions. The Noble team is aware of this regression and intends to address it, so that registration txs are gas-less again. Once that's done, we're unblocked to test again. @zbuc Can you write a detailed test plan in the meantime? Then we can functionally verify this PR is working against latest Noble testnet prior to merge. |
Describe your changes
This introduces changes to rotate Noble addresses based on an incrementing sequence number, as well as renames the
pcli tx register-forwarding-accountcommand aspcli view noble-addressto provide a better experience:Issue ticket number and link
Closes #4873
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: