Jessecar96 / SteamBot

Automated bot software for interacting with Steam Trade
http://scrap.tf
MIT License
1.33k stars 910 forks source link

SteamBot detected item being added twice. #364

Closed waylaidwanderer closed 11 years ago

waylaidwanderer commented 11 years ago

This is the second time that it has happened in one day. Not sure if it was because the Steam network was being buggy, but I'll post the logs:

[Happy Bot 2013-06-26 22:02:53] WARN: Initiating trade with user: [REDACTED]
[Happy Bot 2013-06-26 22:02:55] DEBUG: SteamKit2.SteamFriends+PersonaStateCallback
[Happy Bot 2013-06-26 22:02:56] DEBUG: SteamKit2.SteamTrading+TradeResultCallback
[Happy Bot 2013-06-26 22:02:56] DEBUG: Trade Status: Accepted
[Happy Bot 2013-06-26 22:02:56] INFO: Trade Accepted!
[Happy Bot 2013-06-26 22:02:56] DEBUG: SteamKit2.SteamTrading+SessionStartCallback
[Happy Bot 2013-06-26 22:02:57] DEBUG: SteamTrading.SessionStartCallback handled successfully. Trade Opened.
[Happy Bot 2013-06-26 22:02:58] WARN: I have 40.11 ref in my inventory. 21 ref, 37 rec, 61 scrap.
[Happy Bot 2013-06-26 22:02:58] DEBUG: SteamKit2.SteamFriends+PersonaStateCallback
[Happy Bot 2013-06-26 22:03:01] WARN: User added Refined Metal.
[Happy Bot 2013-06-26 22:03:01] WARN: User added Refined Metal.
[Happy Bot 2013-06-26 22:03:04] WARN: User added Scrap Metal.
[Happy Bot 2013-06-26 22:03:06] WARN: User added Refined Metal.
[Happy Bot 2013-06-26 22:03:06] WARN: User gave me 28 scrap, need to give 9 scrap change.

As you can see, the bot thought the user added 2 refined, 1 scrap, and then another refined for a Name Tag, and so the bot thought it needed to give 1 ref in change. However in reality the user only put up 2.11 ref, as can be seen from the image below.

bclxmvv

So my bot gave out a free refined (although the user reported it and returned it to me).
Earlier, my bot gave out a free scrap when something similar happened.

My question is: is there a way to validate these things? In my validation code I have it double check both Trade.OtherOfferedItems and Trade.steamMyOfferedItems and make sure the value matches, but when things like this occur it seems as though there is no way to catch it.

thesandvichisaliar commented 11 years ago

I only briefly browsed the trading code, but I assume if the bot thought the user added two Refined instead of one, then two of the items in OtherOfferedItems should have the same id, which (unless the item had just been duped) is impossible. A temporary fix, as opposed to a major rewrite involving the extra data proposed in #327, would be to check for duplicate item id's and just abort the trade with an error. This check can be added to ValidateSteamItemChange, and again in ValidateLocalTradeItems, though the latter shouldn't be necessary.

waylaidwanderer commented 11 years ago

That's actually a really good idea. Once I get home from work I will implement that if the tests work out.

iMagooo commented 11 years ago

Yep, my bot does this at least once daily, it's an issue with all of them. Lucky enough, no one has argued to give it back. To be honest, I think that the issue may be on steams end of things. If you look at the trade chat when it happens, it says "You have added Refined Metal" (or whatever) more times than it should have. To me, this indicates that the Steam servers may be reporting the wrong number of events.

It mainly occurs when steam servers are lagging and the user is adding the items in quick succession, although I have been able to replicate it with lag of my own.

More cases of this happening are reported here: #188

I've been too busy studying for my finals to look into it further. If you do get a fix written up (the duplicate ID check idea sounds good to me), I'll be happy to help out with testing it :)

waylaidwanderer commented 11 years ago

I will be testing it out soon.

waylaidwanderer commented 11 years ago

Here's a quick fix I've written up. This is in SimpleUserHandler.cs. I made Trade.steamMyOfferedItems public. To conform with SteamBot's code when/if I make a pull request, I'll probably put it into Trade.cs instead and throw an exception when it happens.

public bool Validate()
{
    if (hasDuplicateID(Trade.OtherOfferedItems) || hasDuplicateID(Trade.steamMyOfferedItems))
    {
        Bot.SteamFriends.SendChatMessage(OtherSID, EChatEntryType.ChatMsg, "Sorry, but a rare Steam network error occurred. Please add yourself to the queue at http://scrapbank.me/banking/ and try again!");
        Bot.log.Warn("Duplicate item was detected; cancelling trade.");
        Trade.CancelTrade();
        return false;
    }

    // ... etc
}

bool hasDuplicateID(List<ulong> list)
{            
    var hashset = new HashSet<ulong>();
    foreach (var id in list)
    {
        if (!hashset.Add(id))
        {
            return true;
        }
    }

    return false;
}
iMagooo commented 11 years ago

I've managed to replicate it once so far with no luck, the trade still went through, managed to buy a key for 4.66 ref (instead of 5.66 ref). The trade is set ready only if Validate() returns true, which it shouldn't if a duplicate exists and obviously, the trade should be cancelled if a duplicate exists. But it wasn't.

I personally don't see any issues in the logic of your code (for testing purposes I only called hasDuplicateID(Trade.OtherOfferedItems)), which leads me to believe that duplicate IDs are not our issue. That is, unless someone else can find an issue with your code.

Edit: This is using your KeyUserHandler, but still, the issue is obviously beyond that.

image

waylaidwanderer commented 11 years ago

How do you replicate it? Just randomly adding/removing items?

This is a different user handler that I'm using, btw.

Maybe log the item ids for each trade? Your logic about duplicate item IDs should be correct, and it's the best thing we have to go on right now.

iMagooo commented 11 years ago

Induce lag and quickly add items, eventually steam reports the wrong amount of items added. In theory your duplicate item fix should work, but it didn't.

I have noticed that while doing this, items sometimes would disappear from the graphical inventory for the duration of the trade. Probably not related in anyway, but still an odd occurrence.

cwhelchel commented 11 years ago

@iMagooo Try this validation (same as waylaidwanderer's validation - just different) in your user handlers 'OnTradeAddItem`:

if (Trade.OtherOfferedItems.Contains(inventoryItem.Id))
{
    // a duplicate item ID was added to the trade. abort
    Trade.CancelTrade();
}
iMagooo commented 11 years ago

Can't try anything at the moment, Steam trade won't even load without lagging out, no idea what happening. Seems to be affecting bots too, lagging out. Nothing wrong with internet connection.

thesandvichisaliar commented 11 years ago

I took a little more time to read and I realize the methods I mentioned only checked the bot's items, though ValidateSteamItemChanged should probably be modified to check for this as well.

A simple check can be performed in trades.cs, while poll() is handling it.

 else
 {      
        If (OtherOfferedItems.Contains(itemID))
             // throw a tradeexception for duplicate id
        OtherOfferedItems.Add(itemID);                                    
        Inventory.Item item = OtherInventory.GetItem(itemID);                                    
        Schema.Item schemaItem = CurrentSchema.GetItem(item.Defindex);                    
        OnUserAddItem(schemaItem, item);                                
 }

I also recommend tracking item ids with debug prints in OnTradeAdd and OnTradeRemove. If there are no duplicate ids, then we must be missing a remove event, but I hope that isn't the case.

I would help test, but I'm travelling and limited to my phone.

iMagooo commented 11 years ago

Alright, so I left the code the same as @waylaidwanderer had suggested and finally the bot has detected a duplicate ID. A trader was trying to sell 7 keys, he admitted he had lag and the bot counted 8 keys. The bot detected the duplicate ID and the trade was cancelled.

@cwhelchel I have also added your validation to the appropriate place. I will report back if any more events occur.

I just have the niggling feeling that it's not always due to duplicate IDs (seeing as I managed to buy the key for 4.66 ref previously with @waylaidwanderer's validation code), I feel that Steam might incorrectly report other IDs added to the trade, which will make it a lot harder to trace.

waylaidwanderer commented 11 years ago

@iMagooo Glad to hear it worked.

seeing as I managed to buy the key for 4.66 ref previously with @waylaidwanderer's validation code

Do you think it was because you were trading it? The condition that has to be met before the bot accepts the trade is if (Validate() || IsAdmin), so is it possible that because you were listed as admin, it skipped the validation?
I suppose we do need to track item IDs like @thesandvichisaliar and I suggested, and only then can we know for sure.

iMagooo commented 11 years ago

@waylaidwanderer It still calls Validate() before it sets the trade to ready, with no IsAdmin condition on that, therefore the trade shouldn't be set to ready unless Validate() returns true. And of course, if Validate() is called with a duplicate ID present, the trade should have cancelled regardless of Admin status.

@cwhelchel Your code is incorrect, it cancels everytime the user adds an item because the code simply tests if the item is offered, not if it has been offered more than once. Placing your code in the Poll() function before the item is added to the list would be a better solution, as @thesandvichisaliar suggested.

cwhelchel commented 11 years ago

@iMagooo Yeah. It should work in Poll. The OnTradeAddItem must be called after it's been placed in OtherOfferedItems so all items will fail that test there.

thesandvich is correct, then, and it should be in Poll. If this works it could be a temporary fix for everyone.

cwhelchel commented 11 years ago

@iMagooo What did you use to induce lag in order to test this? I'm looking at this now with some other code and would like to know what you used.

iMagooo commented 11 years ago

@cwhelchel Nothing advanced, I was simply using uTorrent for some TV shows I had not yet watched >.> haha

iMagooo commented 11 years ago

@redjazz96 @waylaidwanderer @thesandvichisaliar @cwhelchel

Issue not resolved, we simply patched one of the possible issues.

image

Take note, the bot is paying for 5 tickets when only 4 are added. No duplicate is being detected because the duplicate ticket was not added to the trade again!

Jessecar96 commented 11 years ago

As iMagooo said, this issue still exists.

waylaidwanderer commented 11 years ago

You're right, my apologies for closing this issue.

I've implemented a possible fix for my bots at ScrapBank.Me, as can be seen in my comment: https://github.com/Jessecar96/SteamBot/issues/383#issuecomment-20915367 I'm not completely sure if it works, but I will be doing some testing soon.

Jessecar96 commented 11 years ago

Fixed with #384