improvement(ARRR): improve shielded transactions change notes handling in swaps #2331
Conversation
|
tried to swap ARRR with this, but still getting same error |
borngraced
changed the title
|
borngraced
changed the title
|
@cipig can you please retry again? I've made some changes to this PR 🙏🏾 |
borngraced
force-pushed
the
fix-arrr-note-saving
branch
from
f149bb3 to
e735df9
Compare
borngraced
changed the title
|
started a swap with https://sdk.devbuilds.komodo.earth/fix-arrr-note-saving/mm2_e735df9-linux-x86-64.zip
|
|
|
thanks, does this fix the original issue? |
yes, the last commits fixed the issue #2331 (comment) |
|
ARRR change from swaps is not added back to balance |
Let's try the latest build again 9a76c1b Please provide the log file as well. thanks |
mm2src/coins/lp_coins.rs
Outdated
| pub fn new_with_unspendable(spendable: BigDecimal, unspendable: BigDecimal) -> CoinBalance { | ||
| CoinBalance { spendable, unspendable } | ||
| } |
There was a problem hiding this comment.
Just my opinion, not a game-changer blocker:
It doesn't feel right to have these functions as CoinBalance fields are already pubed and it doesn't have too many fields.
The use of
let _ = CoinBalance {
spendable,
unspendable
};seems more clean than
CoinBalance::new_with_unspendable(x, y)because we can't know what is the implementation detail behind new_with_unspendable.
I did a ARRR swap, selling 22 ARRR ARRR balance before the swap was 15762.45617603 The log: |
mm2src/coins/z_coin.rs
Outdated
| #[cfg(not(target_arch = "wasm32"))] | ||
| async fn my_balance_sat(&self) -> Result<u64, MmError<ZcashClientError>> { | ||
| async fn my_balance_sat(&self) -> Result<u64, MmError<ZCoinBalanceError>> { | ||
| let wallet_db = self.z_fields.light_wallet_db.clone(); | ||
| async_blocking(move || { | ||
| #[cfg(target_arch = "wasm32")] | ||
| let balance_sat: u64 = wallet_db.db.get_balance(AccountId::default()).await?.into(); | ||
|
|
||
| #[cfg(not(target_arch = "wasm32"))] | ||
| let balance_sat: u64 = async_blocking(move || { | ||
| let db_guard = wallet_db.db.inner(); | ||
| let db_guard = db_guard.lock().unwrap(); | ||
| let balance = get_balance(&db_guard, AccountId::default())?.into(); | ||
| Ok(balance) | ||
| get_balance(&db_guard, AccountId::default()).map(|v| v.into()) | ||
| }) | ||
| .await | ||
| } | ||
| .map_to_mm(|err| ZCoinBalanceError::BalanceError(err.to_string()))?; | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| async fn my_balance_sat(&self) -> Result<u64, MmError<ZCoinBalanceError>> { | ||
| let wallet_db = self.z_fields.light_wallet_db.clone(); | ||
| Ok(wallet_db.db.get_balance(AccountId::default()).await?.into()) | ||
| Ok(balance_sat) |
There was a problem hiding this comment.
This looks iffy to me. I liked the previous separation.
There was a problem hiding this comment.
I mean not really..
async fn my_balance_sat(&self) -> Result<u64, MmError<ZCoinBalanceError>> {
let wallet_db = self.z_fields.light_wallet_db.clone();
#[cfg(target_arch = "wasm32")]
let balance_sat: u64 = wallet_db.db.get_balance(AccountId::default()).await?.into();
#[cfg(not(target_arch = "wasm32"))]
let balance_sat: u64 = async_blocking(move || {
let db_guard = wallet_db.db.inner();
let db_guard = db_guard.lock().unwrap();
get_balance(&db_guard, AccountId::default()).map(|v| v.into())
})
.await
.map_to_mm(|err| ZCoinBalanceError::BalanceError(err.to_string()))?;
Ok(balance_sat)
}
mm2src/coins/z_coin/z_coin_errors.rs
Outdated
| @@ -170,6 +171,7 @@ impl From<GenTxError> for WithdrawError { | |||
| GenTxError::DecryptedOutputNotFound | |||
| | GenTxError::FailedToGetMerklePath | |||
| | GenTxError::PrevTxNotConfirmed | |||
| | GenTxError::NeededPrevTxConfirmed | |||
There was a problem hiding this comment.
This sounds more like a timeout rather than internal error.
| if tx.block.is_none() { | ||
| if tx.block.is_some() { |
There was a problem hiding this comment.
This changes the behaviour significantly, do we have a test coverage for this?
There was a problem hiding this comment.
The check is very much unneeded and will probably never hit that block of code because only tx that's been mined or included in a block can have a nullifier.
This will return only txs included in block
yep, there's a test here,
..|
|
||
| /// Adds a change note to the tracker. | ||
| pub fn add_change_note(&self, tx_hex: Vec<u8>, change_note: BigDecimal) { | ||
| let mut tracker = self.tracker.lock().unwrap(); |
There was a problem hiding this comment.
Q: Does tx_hex have constant size?
mm2src/coins/z_coin.rs
Outdated
| if changes.is_empty() { | ||
| Ok(spendable_notes) | ||
| } else { |
There was a problem hiding this comment.
The else block is way too large. Can you please do
if changes.is_empty() {
return Ok(spendable_notes);
}and release the else block to improve readability?
mm2src/coins/z_coin.rs
Outdated
| ) -> MmResult<Vec<SpendableNote>, GenTxError> { | ||
| let spendable_notes = self | ||
| .spendable_notes_ordered() | ||
| .await | ||
| .map_err(|err| GenTxError::SpendableNotesError(err.to_string()))?; | ||
| let changes = self.z_fields.change_tracker.change_notes_iter(); | ||
| let my_z_balance = self | ||
| .my_balance() | ||
| .compat() | ||
| .await | ||
| .mm_err(|err| GenTxError::Internal(err.to_string()))?; | ||
|
|
||
| if changes.is_empty() { | ||
| Ok(spendable_notes) |
There was a problem hiding this comment.
If changes is empty, the time spent on my_z_balance will be wasted.
mm2src/coins/z_coin.rs
Outdated
| if balance < change { | ||
| return MmError::err(BalanceError::WalletStorageError( | ||
| "change can't be greater than balance".to_owned(), |
There was a problem hiding this comment.
I am not sure if I am missing something but WalletStorageError doesn't sound like a good fit for this error.
|
In this PR zhtlc-native-tests now work (as in the current dev branch some fail). Could this error be an issue in a non-test environment?) |
|
I believe you want to change the function to async and remove the usage of block_on and await instead @dimxy #[tokio::test] |
Yes I am aware of that (used this option is my PR #2112 months ago), so I ran the test already with [token::test] and 'block_on' replaced on 'await' and was receiving this error. I ran more tests in the zhtlc-native-tests mod with the ZOMBIE chain. 4 of them failed with different errors (at least in my env). |
…d from CoinBalance struct
mm2src/coins/z_coin.rs
Outdated
| let prev = self.z_fields.previous_tx_with_change.lock().unwrap().take(); | ||
| if let Some((tx, _)) = prev { | ||
| info!("Waiting for {tx:?} to be confirmed before next tx can be generated"); | ||
| // Wait up to 30 minutes for confirmations with 15 sec check interval | ||
| // (probably will be changed) | ||
| let wait_until = now_sec() + (30 * 60); | ||
| let input = ConfirmPaymentInput { | ||
| payment_tx: tx.clone(), | ||
| confirmations: self.required_confirmations(), | ||
| requires_nota: self.requires_notarization(), | ||
| wait_until, | ||
| check_every: 15, | ||
| }; | ||
| self.wait_for_confirmations(input) | ||
| .compat() | ||
| .await | ||
| .map_to_mm(GenTxError::NeededPrevTxConfirmed)?; | ||
|
|
||
| info!("Previous transaction confirmed"); | ||
| } |
There was a problem hiding this comment.
Does this mean that we can't spend other unrelated notes when a transaction is unconfirmed, it doesn't make sense. Are we sure this was the issue that caused this #2331 (comment) c.c. @cipig
There was a problem hiding this comment.
Hmm, actually deducting the change from the balance should be already enough if the change from taker_fee won't affect taker_payment.
This is specially for taker fee change to be returned since I assume as taker I don't know when my taker fee is confirmed(if I'm correct) so we need a way to return the change.
Any change created after taker fee e.g taker payment will be returned after confirmation.
There was a problem hiding this comment.
checking if change is needed should invalidate Does this mean that we can't spend other unrelated notes when a transaction is unconfirmed but still where do we know when taker_fee is confirmed?
There was a problem hiding this comment.
Are we sure this was the issue that caused this #2331 (comment)
Yes, indeed. I was able to reproduce it myself
I found out that we can't spend them(change note) right away. This is because in Zcash, every note that you want to spend needs a nullifier - which is basically like a marker that shows if the note has been spent or not.
The problem is that change notes don't get their nullifier until after the original transaction is mined or so. So even though we might want to treat change notes just like regular received notes(the hack in previous PR), we can't, because they're missing this important nullifier until the transaction is confirmed on the blockchain.
I had originally thought we could save change notes as regular received notes, but I was wrong about this because of the missing nullifier issue.
Change notes are unspendable until confirmed
So, I implemented a mechanism that tracks transactions containing change notes and waits for their confirmations before allowing those notes to be used in new transactions.
I also found a bug in WASM walletdb impl which I've fixed in this PR.
cc @shamardy