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

Reproducing examples for the unexpected transfer history bug #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

k0k0ne
Copy link

@k0k0ne k0k0ne commented Sep 10, 2024

No description provided.

@k0k0ne k0k0ne changed the base branch from master to rgb_next September 10, 2024 03:18
@k0k0ne k0k0ne changed the title Reproduce example for unexpected transfer history bug Reproducing examples for the unexpected transfer history bug Sep 10, 2024
@zoedberg
Copy link
Collaborator

This is using an old RGB version. Today I plan to push updated code and merge the initiating PR, after that I'll let you know, so you can rebase this PR there to check the bug is still there.

@dr-orlovsky
Copy link
Member

The tests from @zoedberg are merged so now you can re-base on them

Copy link
Collaborator

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

@k0k0ne I've requested some changes. Also please remember to change the base branch of the PR to master and squash your 3 commits into 1

tests/utils/helpers.rs Outdated Show resolved Hide resolved
tests/history-missing-bug.rs Outdated Show resolved Hide resolved
tests/history-missing-bug.rs Outdated Show resolved Hide resolved
tests/history-missing-bug.rs Outdated Show resolved Hide resolved
@k0k0ne k0k0ne changed the base branch from rgb_next to master September 18, 2024 04:09
@k0k0ne
Copy link
Author

k0k0ne commented Sep 18, 2024

I have modified the code as requested. Please check it.

Copy link
Collaborator

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

@k0k0ne please see the requested changes

tests/transfers.rs Outdated Show resolved Hide resolved
tests/transfers.rs Outdated Show resolved Hide resolved
tests/transfers.rs Outdated Show resolved Hide resolved
tests/utils/helpers.rs Outdated Show resolved Hide resolved
tests/utils/helpers.rs Outdated Show resolved Hide resolved
@dr-orlovsky
Copy link
Member

But @zoedberg do you confirm that the logic of the test is correct and there is a bug in the history showcased by the test?

@k0k0ne
Copy link
Author

k0k0ne commented Sep 19, 2024

I have modified the code as requested. Please check it.

@dr-orlovsky
Copy link
Member

The compilation fails due to error: unused import: WalletError`` (see https://github.com/RGB-WG/rgb-tests/actions/runs/10936428253/job/30361992550?pr=3#step:5:54)

Can you please make sure it compiles? Also running cargo lint --all-targets should help

tests/utils/mod.rs Outdated Show resolved Hide resolved
@zoedberg
Copy link
Collaborator

But @zoedberg do you confirm that the logic of the test is correct and there is a bug in the history showcased by the test?

Without investing the time to dig deeper, it's not entirely clear to me what the test should achieve and what fungible_history should return. @dr-orlovsky, since you know fungible_history way better than anyone else, can you please look into this?

@dr-orlovsky
Copy link
Member

fungible_history should return a history of fungible asset operations. Each operation - a raw in a history, either "paid-in" or "paid-out". From what I see the test logic is correct, but I'd like your confirmation since you understand better what each call means. For instance, I do not get false in mine(false) etc

@zoedberg
Copy link
Collaborator

For instance, I do not get false in mine(false) etc

As answered in private:
That boolean means resume (mining), you need to put it to true in case a test stops the mining (with the stop_mining method) and then (usually after performing some checks/tasks) wants to allow mining to work again. Stopping mining prevents other tests to mine during a phase where it's important that no new blocks get mined.

@dr-orlovsky If there are other things you don't understand feel free to ask me

Anyway, the check_fungible_history is very verbose but what it does (before the panic occurs) is:

  1. wlt 1 issues 100000 of a NIA asset
  2. wlt 1 sends to wlt_2 200 of that asset (in witness mode)
  3. wlt 2 accepts the consignment
  4. a block gets mined and both wallets perform a sync

after point 1, the test checks that fungible_history returns 0 results (not sure if this should be the case)

after point 4, the first check passes (it checks that wlt 1, the sender, has a Dec(Amount(200))), while the second check fails because wlt 2, the recipient, expects a state_change of Inc(Amount(200)) but it instead contains Dec(Amount(99800)):

thread 'check_fungible_history' panicked at tests/transfers.rs:837:5:
assertion `left == right` failed
  left: Dec(Amount(99800))
 right: Inc(Amount(200))

this is the part I'd like you to tell me if it's a bug or not, since it's still unclear to me what the method should return

Speaking instead about the send_to_oneself test:

  1. wlt 1 issues 100000 of a NIA asset
  2. wlt 1 sends 300 of that asset (in witness mode) to itself
  3. wlt 1 accepts the consignment
  4. a block gets mined and the wallet performs a sync

after point 4, the test checks that the history is not empty (which succeeds)

@k0k0ne the test passes so it's unclear what bug this test should show

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Sep 19, 2024

this is the part I'd like you to tell me if it's a bug or not, since it's still unclear to me what the method should return

Wlt 2 never had 100000 assets and the only thing it ever had is 200 assets received. So this looks like a bug. But I'd like to see is this all of the history or maybe Inc(200) goes after it as a second record.

@dr-orlovsky
Copy link
Member

I confirm the bug, I am working on a fix. @k0k0ne thank you very much for providing the demonstration of the case.

Please also let us know whether the second test demonstrate some other issue - or it is done just as a check

@k0k0ne
Copy link
Author

k0k0ne commented Sep 19, 2024

The second test appears to be working after updating the environment (unlike with previous version).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants