improvement(ARRR): improve shielded transactions change notes handling in swaps #2331
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 |
onur-ozkan
reviewed
Feb 3, 2025
mm2src/coins/lp_coins.rs
Outdated
Comment on lines
2455
to
2457
| 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.
This tx is okay by the look of it (absence of transparent vins is normal if a tx has a shielded spend). |
the debug.log of ARRR on the electrums does not contain "4498a4534bad9e0406822ad195bce306baa862a4f4bae3f469bbfd70ac8e88c7" found this entry but it's a different txid |
BTW Looks like the librustcash sets DEFAULT_TX_EXPIRY_DELTA to 20 whereas the komodo code sets it as 200. Should not it be changed for us @borngraced? |
it's a different tx. No more 'CheckTransaction' logs? |
but I don't think the expiry height is causing the current bug(20/200 expiry looks long enough) and I can confirm the issue is no longer related to locked note @cipig can you please make a swap with all notes to stimulate |
Not sure what you mean... this swap #2331 (comment) was done with latest mm2 from this branch as you can see it didn't wait for confirmation of takerfee... it tried to send takerpayment while takerfee had 0 confirmations |
it's not going to wait for
So if |
OK, then it means the reason why those 2 swaps failed is something else. |
|
did a swap with a different ZHTLC coin, which failed with shows that mm2 indeed waits for takerfee to be confirmed before it sends takerpayment this is the entire log file... look at the end... the new ZHTLC coin has the ticker ZDEEX question: why does it wait for 3 confirmations on takerfee? one would be enough to see the change... |
|
tried a new swap, also with ZDEEX
idk if it's related to this PR, i guess it's not... but thought i mention it anyway logfile... same as the one from previous post, but with the new swap |
yes, this is indeed the expected behavior. Maybe we can increase the timeout in this case(the current timeout I set is relatively too short). Also, I will make it 1 confirmation. |
onur-ozkan
reviewed
Feb 19, 2025
Comment on lines
+1206
to
+1207
| async fn clean_up(&self, _uuid: Uuid) -> MmResult<(), String> { Ok(()) } | ||
|
|
There was a problem hiding this comment.
It's worth to document this I think.
Comment on lines
+79
to
+88
| Ok(db | ||
| .call(move |conn| { | ||
| conn.prepare(&format!( | ||
| "INSERT OR REPLACE INTO {table_name} (hex, hex_bytes, change) VALUES (?, ?, ?)" | ||
| ))? | ||
| .execute(params![hex, hex_bytes, change])?; | ||
|
|
||
| Ok(()) | ||
| }) | ||
| .await?) |
There was a problem hiding this comment.
Maybe map the error instead of using ? and then wrapping it with Ok? It seems badly formatted this way. There are similar other cases in this PR.
Comment on lines
+13
to
+14
| let table_name = table_name(for_addr); | ||
| validate_table_name(&table_name)?; |
There was a problem hiding this comment.
not a blocker: Can we perform the validation directly in table_name? If we forget calling validate_table_name (which is quite possible), it will be security concern.
dimxy
reviewed
Feb 19, 2025
| } | ||
|
|
||
| Ok(CoinBalance { | ||
| spendable: &spendable - &unspendable, |
There was a problem hiding this comment.
Is this always correct calculation?
I think, when we just sent a tx with a change output the my_balance_sat fn (which queries the received_notes table) does not include this tx change note.
It would include it only when the tx is confirmed and the block is processed.
There was a problem hiding this comment.
spendable includes notes from unconfirmed tx
dimxy
reviewed
Feb 19, 2025
| use mm2_core::mm_ctx::MmArc; | ||
| use mm2_err_handle::prelude::*; | ||
|
|
||
| fn table_name(addr: &str) -> String { format!("{addr}_change_notes_cache") } |
There was a problem hiding this comment.
Here we segregate change notes by addresses.
But the spendable balance obtained in get_balance fn is queried by the AccountId. Should this not be done identically?
Also, why not just have a column with the address instead of adding it to the table name?
dimxy
reviewed
Feb 20, 2025
|
|
||
| Ok(CoinBalance { | ||
| spendable: &spendable - &unspendable, | ||
| unspendable, |
There was a problem hiding this comment.
Yet another remark about the 'unspendable' concept:
Here we include only 'change' into the 'unspendable' balance. But there are also unconfirmed received notes, which we do not track. So users may wonder, why we show the unconfirmed change as 'unspendable' but do not show recently received outputs like that.
I took a look at the latest librustzcash version - they separate unspendable change and received value as different vars in the balance.
There was a problem hiding this comment.
I'm not sure if we we still need to wait for additional confirmations after we received the notes(change) back to our wallet.
I think once our wallet sees the change note in a confirmed block, it becomes available for spending(usually after the first confirmation) hence why get_spendable_notes return it as part of our balance.
|
I am running zombie tests (test need to be fixed though) and getting errors when all the tests are run together: The errors are from sendrawtransaction: I traced the komodo code and found locations where errors are generated: Looks like both errors are related to reused nullifies. |
Those are indeed the same errors i got on my failed swaps, either |
This may happen during only one swap (I am seeing this in the trade_test_electrum_rick_zombie test). |
|
the builds from this branch were removed from https://sdk.devbuilds.komodo.earth/ |
Done @cipig |
dimxy
reviewed
Mar 7, 2025
| // Wait for tx confirmation | ||
| // Wait up to 30 minutes for confirmations with 15 sec check interval | ||
| // (probably will be changed) | ||
| let wait_until = now_sec() + (30 * 60); |
There was a problem hiding this comment.
I think maybe it's better not to do waiting inside the KDF and just delegate this to the User: simply return unconfirmed balance to the GUI (GUI may display unconfirmed balance and a warning that confirmation is needed)
There was a problem hiding this comment.
the is the same as gui making sure that there's no more than one ARRR swap running..right?
|
tried to do 2 ARRR to EURE swaps and both failed on takerpayment: |
first one failed with the familiar error (on a PIRATE node): Still note reuse, I guess |
|
third swap worked fine, also ARRR to EURE |
I have additional fixes lined up that should limit the occurrence of failing txs due to this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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