search

metaplex-foundation / metaplex-program-library

Smart contracts maintained by the Metaplex team
Other
588 stars 513 forks source link

[Bug]: Execute sale - sum of account balances before and after instruction do not match #822

Closed viyer28 closed 1 year ago

viyer28 commented 2 years ago

Which package is this bug report for?

auction-house

Which Type of Package is this bug report for?

Rust Contract

Issue description

Running into a weird error with executeSale - I'm trying to execute a sale as the seller of a token on an existing buy offer. Basically, I'm packing createSellOffer, printListingReceipt, executeSale, and printPurchaseReceipt in one transaction. In the executeSale instruction, I get this error:

sum of account balances before and after instruction do not match

It looks like something to do with ImmutableOwner and Token Program 2022. I've attached a sample failed devnet transaction here.

Any idea what could be going on?

Relevant log output

> Program log: Instruction: ExecuteSale
> Invoking System Program
  > Program returned success
> Invoking System Program
  > Program returned success
> Invoking System Program
  > Program returned success
> Invoking System Program
  > Program returned success
> Invoking Associated Token Account Program
  > Program log: Create
  > Invoking Token Program
    > Program log: Instruction: GetAccountDataSize
    > Program Token Program  consumed 1622 of 637426 compute units
    > Program return: TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA pQAAAAAAAAA=
    > Program returned success
  > Invoking System Program
    > Program returned success
  > Program log: Initialize the associated token account
  > Invoking Token Program
    > Program log: Instruction: InitializeImmutableOwner
    > Program log: Please upgrade to SPL Token 2022 for immutable owner support
    > Program Token Program  consumed 1405 of 630936 compute units
    > Program returned success
  > Invoking Token Program
    > Program log: Instruction: InitializeAccount3
    > Program Token Program  consumed 4241 of 627052 compute units
    > Program returned success
  > Program Associated Token Account Program  consumed 23545 of 646018 compute units
  > Program returned success
> Invoking Token Program
  > Program log: Instruction: Transfer
  > Program Token Program  consumed 4735 of 613710 compute units
  > Program returned success
> Program Auction House  consumed 104003 of 709593 compute units
> Program returned error: sum of account balances before and after instruction do not match

Priority this issue should have

High (immediate attention needed)

gabspeck commented 1 year ago

Hi, I have encountered this issue as well. Someone on the Discord channel suggested that this is due to some bug in executing sales using the seller's key.

The workaround suggested, and that worked for me, was enabling requireSignOff in the auction house instance and then adding the auction house's authority as a signer to the instruction. This may or may not be viable to you, since requireSignOff will require all of the AH transactions to be signed by your auction house authority.

AmmarKhalid123 commented 1 year ago

@gabspeck what should one do if turning requireSignOff on is not viable in my use case, bcs I can't really do that

boredjoker commented 1 year ago

any update?

AmmarKhalid123 commented 1 year ago

@austbot @blockiosaurus any suggestions on this?

crypt0miester commented 1 year ago

double check that the fee payer has more than needed to execute the txn. since there are a couple of accounts created when using the route you guys have gone through.

~would also be helpful to include a txn id on devnet.~ I saw @viyer28's txn. it seems that you would need to deposit more than the actual price of the sale. how much exactly? no idea.

AmmarKhalid123 commented 1 year ago

In the escrow? bcs in escrow we have to enter the exact amount while creating the instruction using metaplex-foundation And if you're talking about seller's wallet (which is executing the transaction and will be the fee payer of all txs in this case), they already have around 0.1 sol extra in their wallet which ig should be enough

Also, if it might help, executeSale works fine when the flow is: Seller lists for x sol (sell, sellListingReceipt) and then buyer comes and buy (bid, bidReceipt, executeSale, executeSaleReceipt)

crypt0miester commented 1 year ago

Also, if it might help, executeSale works fine when the flow is: Seller lists for x sol (sell, sellListingReceipt) and then buyer comes and buy (bid, bidReceipt, executeSale, executeSaleReceipt)

sounds like the intended right flow, when requireSignOff is false.

two things you can also try:

see if that works for you.

AmmarKhalid123 commented 1 year ago

Have tried both of them, doesnt work

viyer28 commented 1 year ago

@crypt0miester I've tried manually adding funds to the sellerTradeState too, since the seller is the payer in this case. that also didn't seem to work.

what's strange to me is this exact flow was working normally around a month ago, it was a recent version of auction house that caused it to break

crypt0miester commented 1 year ago

@crypt0miester I've tried manually adding funds to the sellerTradeState too, since the seller is the payer in this case. that also didn't seem to work.

what's strange to me is this exact flow was working normally around a month ago, it was a recent version of auction house that caused it to break

ez pz. find the working contract and deploy it. good luck.

AmmarKhalid123 commented 1 year ago

ez pz. find the working contract and deploy it. good luck.

I traversed the commits which updated the AH, and this last commit on Auction House, i.e., https://github.com/metaplex-foundation/metaplex-program-library/tree/380266975aafeb00f25016e119f881d9c3f437e3/auction-house The tests for execute_sale are failing in this commit, before that its working. So I'm guessing this one must be creating some issue @crypt0miester

AmmarKhalid123 commented 1 year ago

@crypt0miester deployment on devnet took around 17-18ish sol, which would be a bit too costly on mainnet :( Any chance this issue will be fixed in near future?

crypt0miester commented 1 year ago

@crypt0miester deployment on devnet took around 17-18ish sol, which would be a bit too costly on mainnet :( Any chance this issue will be fixed in near future?

prune auctioneer functions off. I have no idea what will happen in the future. kek

I'm just trying to help you guys.

AmmarKhalid123 commented 1 year ago

Thank you @crypt0miester, it worked. @blockiosaurus @austbot @levicook This is a verified bug. Reverting code to 90faa3459b596021b1d734bffedd83893256a981, deploying the contract, and then using that programId resolves the issue. This should be a high priority task, as in solscan I cannot see any successful executeSale executed by a seller too, so it must be affecting alot of people out there. I don't have much knowledge of rust else I would've looked into it to resolve the issue

crypt0miester commented 1 year ago

Thank you @crypt0miester, it worked. @blockiosaurus @austbot @levicook This is a verified bug. Reverting code to 90faa34, deploying the contract, and then using that programId resolves the issue. This should be a high priority task, as in solscan I cannot see any successful executeSale executed by a seller too, so it must be affecting alot of people out there. I don't have much knowledge of rust else I would've looked into it to resolve the issue

the commit you linked only has bubblegum changes. weird.

deploy the idl and apr. iirc, explorer.solana.com checks for idl data.

and you are most welcome. :D

update: so I took my time to check for changes in mpl_auction_house commit. and the only change prior to that commit you linked is this can you please double check if that is the case?

AmmarKhalid123 commented 1 year ago

@crypt0miester Umm, I guess we should look for the commits that came after 90faa3459b596021b1d734bffedd83893256a981 (29 Sept) since the flow is working fine on this commit, and below is the list of commits that made some changes to auction-house programs and occurred after the mentioned commit

f448a128c0db027501e5c5097f1d46e28cf88757 (updates execute_sale/mod.rs) 03ad5bd083f930f4828276317c8085515efc2d14 (doesn't update AH programs, just the js files) 471d3792b755ffe76e0425b41f25b9c3f70cd182 (updates utils.rs) ac347fb4c5dec6e32f3e62ff3a87cdfa016fb378 (doesnt update programs, just the js sdk) 40c0df078ca14b5240897b4c4259d31050ba6acd (update execute_sale/mod.rs) bcc37d6c51bd1adeb889212bcc750df9fdedfab3 (doesnt update programs, just the sdk) 380266975aafeb00f25016e119f881d9c3f437e3 (this is the latest commit, and the tests for execute_sale are failing here)

That's the only thing I could think of which might help to whoever comes across this and have anchor/rust knowledge. The changes in any one of the above is breaking the flow

crypt0miester commented 1 year ago

@crypt0miester Umm, I guess we should look for the commits that came after 90faa34 (29 Sept) since the flow is working fine on this commit, and below is the list of commits that made some changes to auction-house programs and occurred after the mentioned commit

f448a12 (updates execute_sale/mod.rs) 03ad5bd (doesn't update AH programs, just the js files) 471d379 (updates utils.rs) ac347fb (doesnt update programs, just the js sdk) 40c0df0 (update execute_sale/mod.rs) bcc37d6 (doesnt update programs, just the sdk) 3802669 (this is the latest commit, and the tests for execute_sale are failing here)

That's the only thing I could think of which might help to whoever comes across this and have anchor/rust knowledge. The changes in any one of the above is breaking the flow

so they moved the closing of buyer_trade_state from inside of the token_account_data.amount check in commit that you mentioned to outside the check latest commit

I believe this reflects the latest changes in token_metadata program, since the check is changed from amount to delegated_amount.

perhaps moving it back to line 1791 inside the check would make it work. no idea of the implications though. so make sure the rust tests go through.

AmmarKhalid123 commented 1 year ago

perhaps moving it back to line 1791 inside the check would make it work. no idea of the implications though. so make sure the rust tests go through.

Makes sense. Currently occupied with few things, can try this out in a day or two. Will update here if the solution works

blockiosaurus commented 1 year ago

Thanks for catching this and pinning it down to a commit! It looks like we're aware of this bug but it got lost in the pre-Breakpoint shuffle. I'm on vacation this week so will ping the on-call engineer again to make sure they follow up.

samuelvanderwaal commented 1 year ago

Hi all, apologies for the delay in addressing this. I'm struggling to duplicate this error in Rust BPF tests. Here's what I'm doing, please advise if this isn't properly duplicating the flow that is causing the error:

I've tried this test on several different commits and it's worked on all of them so far.

AmmarKhalid123 commented 1 year ago

@samuelvanderwaal try buy, not the public_buy.

AmmarKhalid123 commented 1 year ago

just tried, using public_buy with this flow also throwing same error :/

samuelvanderwaal commented 1 year ago

just tried, using public_buy with this flow also throwing same error :/

Both buy and public_buy work with my Rust BPF tests so I'm not duplicating your flow somehow. I'll try setting up a test with a CLI on devnet tomorrow so we can compare transactions. Thanks for your patience.

viyer28 commented 1 year ago

40c0df0 (update execute_sale/mod.rs)

My hunch is it has something to do with this commit.

AmmarKhalid123 commented 1 year ago

@crypt0miester @viyer28, thanks for the suggestions. I tried to make the changes as suggested by you two, and deployed on devnet (ProgramId: 3D9cn2367JxcHyKPQpySFoZgf2ZDekh9ZVfEreVWT4WA). Tested with the same flow which was causing the issue on ExecuteSale for @viyer28 and me too, but is working fine. Just in case any of you wants to test it, below is the auction house keys which I created using this ProgramId and tested the flow on. { "authority": "rHETQthTcwEVwHCxj3YDGQcJCyFxCAn5QQmbgnMjYzq", "payer": "rHETQthTcwEVwHCxj3YDGQcJCyFxCAn5QQmbgnMjYzq", "treasuryMint": "So11111111111111111111111111111111111111112", "feeWithdrawalDestination": "rHETQthTcwEVwHCxj3YDGQcJCyFxCAn5QQmbgnMjYzq", "treasuryWithdrawalDestination": "rHETQthTcwEVwHCxj3YDGQcJCyFxCAn5QQmbgnMjYzq", "treasuryWithdrawalDestinationOwner": "rHETQthTcwEVwHCxj3YDGQcJCyFxCAn5QQmbgnMjYzq", "auctionHouse": "21aSox4W7C72XVdeyfnecgJ4xS8TmUXbU3P1UxEQL4uS", "auctionHouseFeeAccount": "5GZRUuX6sds78ntTyBmf4mrQAqNQANsgmGruGbHwWBvu", "auctionHouseTreasury": "3dtumsMMWHDWzuVdzbuQnFcQ5PFFLMw9HHZ1Z1LUugrH" }

AmmarKhalid123 commented 1 year ago

@samuelvanderwaal can you take a look at this PR #871

crypt0miester commented 1 year ago

@samuelvanderwaal can you take a look at this PR #871

glad you got it working mate.

AmmarKhalid123 commented 1 year ago

@crypt0miester wouldn't have been possible without your help though. Let's hope it gets approved.

samuelvanderwaal commented 1 year ago

@AmmarKhalid123 I've deployed a fix to devnet. Please let me know if this resolves the issue for you.

AmmarKhalid123 commented 1 year ago

@samuelvanderwaal yes it does, working on devnet now. Thank you. Can you deploy the fix to mainnet too?

samuelvanderwaal commented 1 year ago

@samuelvanderwaal yes it does, working on devnet now. Thank you. Can you deploy the fix to mainnet too?

The fix is on mainnet now. Please let us know if you see any more issues with this.