2 apparent beancounter bugs causing missing tracking entries
  • Hi,
    I would have logged bugs in JIRA, but I see that non team members can't do that. So here I am. Hopefully someone will see this :)

    I noticed a while ago that beancounter occasionally did not record certain of my failed auctions. It took me a while but I think I've determined two reasons for this:

    1. For certain items with the newer random enchants (negative suffixIds), the unique id that gets returned in the mail invoice does not match the original unique id of the posted auction.
    This is described in some detail in this wow wiki page:
    http://www.wowwiki.com/ItemString
    Draw your attention to the part talking about Unique ids and negative suffixIds.
    Fortunately I think I've found a way to 'reverse engineer' the original uniqueid of the posted auction by applying the bit.band algorithm mentioned in that wiki page, as follows:

    in the findStackfailedAuctions function, as it's scanning the postedAuctions table, calculate the suffix factor of both the postedAuction uniqueId as well as the incoming mail item suffixFactor using the suffixId and bit.band algorithm.

    Then, to get the original uniqueid of the mail item,
    adjustedUniqueId = incomingUniqueId - incomingSuffixFactor + currentSuffixFactor

    Since not all items have this problem when going through the mail, it is then necessary to check both the mail's incoming uniqueid as well as the adjustedUniqueId (in case the incoming one didn't match) against the posted auctions for a match.

    I have hacked my local copy of the addon to get this working... and so far it seems to do the trick.


    2. Another problem, after fixing the above.. sometimes items were still not getting tracked.
    In my case it was because another addon was reading messages before beancounter got a chance to.
    I tracked the problem down to the hook that was added, PreGetInboxTextHook.
    In this function, if the sender and subject was available and the message was not yet read, then the hook records an 'override' that gets used later in updateInboxStart().
    In my case this was not working, because in the case where subject and/or sender were empty, the other addon would go ahead and read the message anyway, causing wasRead to become true. But no override would be recorded in that case.
    Then in updateInboxStart(), even after the sender and subject become available, since wasRead was now true and no override was recorded, the function would skip that message and never record the auction result.
    After looking at this for a while, I am really not sure why that hook is even required. What purpose does it really serve?
    Even if beancounter reads the same message multiple times, the first time it does so it removes the matching auction from the BeanCounter data, so subsequent attempts to read the message result in no match, and it errors out. If you have debug turned on it seems like something is wrong, but it actually seems to work better that way than if the hook was enabled!
    Personally, I think it might work better if Beancounter kept track of what *it* reads, instead of worrying about what other addons are reading.


    Unfortunately I am a novice when it comes to Lua and writing addons, so all I have right now is an ugly hack. But if anyone has any questions about what I wrote above, or wants to see my hack job, please let me know. I am happy to try to help.
    I actually am a developer for a living, but like I said... not good with Lua yet :)
  • Ok, for #2 above actually it's not so great if Beancounter reads the message multiple times. Total read counts end up being off. So I guess other changes would need to be made besides what I've done already.. I'll keep thinking about it :)
  • Thanks for your comments :)

    The first bug is somewhat known:
    basically it affects all suffixed items that originally had positive suffixes - they were all changed to have negative suffixes. I think this only applies to Classic tier random items.

    When such an item is in your bag or on the AuctionHouse it has the factor correctly embedded in the lower 16 bits of the UniqueID, as detailed in the link you posted.
    The problem is that when it is mailed the UniqueID changes, presumably to some 'hidden' internal ID, so this is really a Blizzard bug.
    Also it affects new items, not just items obtained before MoP - you can confirm this by farming a Classic zone til you get a suffixed item drop, make a note of its UniqueID, mail it to an Alt, check the UniqueID while it's in the Mailbox.

    It's something that should be fixable, one way or another - note that BeanCounter is able to match up sold items without an itemlink at all! Unfortunately our BeanCounter Dev is MIA, so we don't have anyone working on it right now.
  • Ah, that's a shame that the developer is MIA. Well, thanks for the response!
  • If someone can come up with workable fixes, we can stick them in. I think I'd prefer to keep these two things as separate issues though, especially if you're getting side effects from the second one.

    For the first, it sounds like you are saying only the low 16 bits changes? So rather than calculating the suffixes and subtracting/adding, maybe you could mask to keep the high 16 bits and compare those instead?
  • Hi,

    I agree, separate issues makes sense.

    For your suggested fix for the first one, that may work. I'm just not sure how unique the high 16 bits alone would be... perhaps it's sufficient. I don't know the internals of WOW well enough to judge.

    For the second, I think I've come up with some changes that allow the hook to work better. But my copy is a bit messy now. I'm noticing some repeated code in this addon too, maybe it could be refactored a bit.

    I'm still testing this out to see if it really holds up over more scenarios.
  • Hi, I've created two patches, one for each of the issues.

    Note: The patches were created for the current "prod" version, 5.19.5445. I have not tried these with the current stuff in the trunk.

    How should I submit these?
  • if you upload them somewhere and post links, we can get to them.

    alternately, if you might be interested in actually joining the dev team, pop into our #norganna irc channel and ask about joining, and I'll try to get MP to set up with with dev credentials.
  • Ok. Here are a couple of gists containing the diffs.
    These were created using git, as per the instructions here: http://scribu.net/wordpress/svn-patches-from-git.html

    Hopefully you will be able to use them without any trouble. Let me know...

    Patch for issue # 1 above:
    https://gist.github.com/eehret/11122694

    Patch for issue # 2 above:
    https://gist.github.com/eehret/11122781

    Thanks
  • Thanks, I'm trying out #1, and it did patch with no trouble :)

    Looking at it, I realize I was thinking of a different aspect of the same problem: When you bid on an auction, BeanCounter wasn't recognizing the item when you won it.
    Although similar, looks like that will need a slightly different solution.

    Still, will go farm some stuff to test posting - luckily low level gear isn't too expensive to post when you want it to fail... posting at 12 hours should be cheaper though.
  • Ah yes. I didn't deal with won auctions, never noticed a bug there.
  • OK, Results:

    I posted 6 suffixed items for 12 hours; 2 of them sold (apparently I didn't hike the price up enough...), of which only 1 was recorded (not related to the #1 fix, but will need looking into later).

    3 others came back to me and did get tagged as Failed.
    However, when I looked into the save file, I noticed a problem: in "failedAuctions" those entries had odd values for the uniqueID of their strings, which were the bad (mailbox) itemIDs ANDed with 65535.

    Look at function databaseAdd in BeanCounter.lua (~516).
    For certain categories it does a 'compress', which includes removing the uniqueID, or converting it to just the suffix factor.

    So in BeanCounterMail, we also need to pass the correct itemLink to databaseAdd, when appropriate.


    Oh, one other little thing, using i:gsub(incomingUniqueId, adjustedUniqueId) : I'd be caustious about doing it this way, in case incomingUniqueId unexpectedly matched something else in the string, and replaced that as well...

    edit: I accidentally posted one for 48hrs - so cancelled it and looking at how it gets recorded: same problem with the suffix factor as for failed auctions.
  • Hmmm interesting. I did not notice that. Failed auctions seemed to be getting recorded correctly for me.

    Just took another look at the code, it doesn't compress in the case where the suffixID is negative...which is the case that my patch was trying to address. SuffixID is negative for random enchants. Were you seeing that item strings were not being compressed for cases where SuffixID was >= 0?
  • Regarding gsub, you're right. Although I would think it's highly unlikely that the whole uniqueid will just happen to match something else :)
  • databaseAdd does compress for negative suffixes - it calculates the scaling factor by ANDing with 65535.
    Unfortunately, if we get a problem link from the mailbox, this is exactly the part that is wrong!

    I'm not sure what problems this would cause, although it looks like one reason databaseAdd strips the uniqueID is so as to group together otherwise identical items - this will fail when we have some random bits instead of the real scale factor.
  • Hmm, I'm not sure which code you're looking at. It seems different from what I saw. I could be wrong - I will take another look later.
  • Ok, I took another look. I see what you are trying to say. I thought by compress you meant replacing with 0. Yes, it does extract the scaling factor and store the resulting uniqueid. This part I did not modify. I don't really consider it "compressing" because the item string isn't shorter in that case. But now we're getting into semantics.

    I still don't see though what you think is going wrong. You said the unique ids had "odd" values. What do you mean?


    I should also mention that I was trying to patch around the existing code that seemed to be working. I didn't want to risk breaking other things. So I did not modify the databaseAdd portion or most of the rest of the code. I only added the part that computes the modified uniqueid on the incoming mails to try to improve the matching.

    Now, it may be that there is a much better way to fix the problem by making more extensive modifications. I thought about that, but didn't have the time, nor the experience with LUA to do so confidently. Hopefully the patch has helped to at least clearly identify some problem cases, and one potential approach to solve it.
    One challenge is that we don't know what the "wrong" uniqueids are until getting them back in the mail. I could not record it when the message was sent TO auctionhouse. I wasn't sure how to get around that, hence my weird id computing step :)

    I still believe this patch does at least work properly, at least for failed auctions (the case I was focussed on fixing). I have been using it for 2+ months and have not found any more items that are still missed since patching.

    I will continue using it until someone does a better fix :)
  • Errrr I think I finally get what you are trying to tell me.

    Are you suggesting that we record the correct itemid in sortFailedAuctions instead of the one we got back from the mail?

    That should be possible, since I would have found out what the correct one was after getting a match. I still don't understand how that makes a difference, but I'm willing to work on that if your dev is still AWOL :)
  • Ahhh, was looking over the code again, and spotted something, ~361:
    local _, _, _, _, _, _, _, dbSuffixId, dbUniqueId, _, _, _ = strsplit(":", i)
    local currentSuffixFactor = lib.API.getSuffixFactor(dbSuffixId, dbUniqueId)
    local adjustedUniqueId = tonumber(incomingUniqueId) - tonumber(incomingSuffixFactor) + tonumber(currentSuffixFactor)
    local adjustedItemString = i:gsub(incomingUniqueId, adjustedUniqueId)

    I think it should be
    local adjustedItemString = itemString:gsub(incomingUniqueId, adjustedUniqueId)

    Otherwise adjustedItemString will always be set to i
    And the following test will always succeed when it reaches or i == adjustedItemString


    But yes, I was suggesting that if findStackfailedAuctions detects that the uniqueID is wrong, it passes the corrected one (found in i) back to sortFailedAuctions, which then passes it on to databaseAdd.
    Needs some care, though, as i is an itemString, whereas databaseAdd gets called with the full itemLink. I think this can be worked round (without having to construct a corrected full itemLink), need to look at the code some more...

  • I'll describe what I found from testing, to show why databaseAdd needs to be sent the corrected uniqueID:

    My test posting items included 3 different 'Medicine Staff'. I looted them on an Alt, so I got the chance to make a note of the correct uniqueIDs (from my bags), and the 'bad' uniqueIDs (when I mailed them to my Auctioneer).

    Medicine Staff (itemID 4575) will always have a negative suffix, and will always have a suffix factor of 6.

    I'll just detail one example:
    Medicine Staff of Intellect (-19)
    correct uniqueID 464060422 (1BA90006 hex)
    mail uniqueID 464120192 (1BA9E980 hex)

    Extract from my save file:
    ["4575"] = {
    ["item:4575:0:0:0:0:0:-9:9728:80:0"] = {
    "1;;222;;40000;40000;;1398466728;;H", -- [1]
    },
    ["item:4575:0:0:0:0:0:-69:57344:80:0"] = {
    "1;;888;;3567494;3210745;;1398517242;Cancelled;H", -- [1]
    },
    ["item:4575:0:0:0:0:0:-19:59776:80:0"] = {
    "1;;222;;39556;35600;;1398466698;;H", -- [1]
    },
    },

    As you can see, the matching (-19) entry has a 'uniqueID' of 59776 (E980 hex), i.e. the low 16 bits of the mail uniqueID.
    It should be 6 (and so should the others).
  • Thanks for the details. I'll take another look :)
  • Looked at the code again, and I think it is safe to call databaseAdd with just the itemString (i.e. i):

    databaseAdd accepts either full itemLink (parameter 2) or the itemString (parameter 3). (Or both.)
    databaseAdd works the same in both cases, except that if a full itemLink is provided it will additionally call API.storeItemLinkToArray at the end.

    I think the call to API.storeItemLinkToArray is unnecessary here, as it should have been called for this link on a previous occasion (when the entry was first added to postedAuctions).
  • I've got time now to look at this again -

    Looks like passing the corrected itemString back down and on to databaseAdd will work.

    I think that your getSuffixFactor function may be a bit too specialized to go into API, it's a helper function a bit like private.matchDB, so should probably go in the same place. I also had some ideas to shift some of the duplicated code into it - and out of findStackfailedAuctions/findStackCancelledAuctions.

    Another thought, re:

    if i:match(itemString) or i == itemString or i:match(adjustedItemString) or i == adjustedItemString then

    I don't think the two match tests are necessary.
    In fact, for the items with a negative suffix that we are trying to fix, i:match(adjustedItemString) should always fail, since adjustedItemString contains a '-' character, which has special meaning in string.match.

  • @analogkid76:
    I've Commited what is essentially your fix, with minor changes as I mentioned above, r5466.
    See http://jira.norganna.org/browse/BCNT-345

    Looking at issue #2, as http://jira.norganna.org/browse/BCNT-347
    I can see why it was originally written the way it was, and I think I see what you've done to change it, and looks OK.
    As you said, it would be a lot better to track what BC has read, rather than what other AddOns are reading - maybe in the future...

    Something else cropped up though - after I sold off those 'Medicine Staff's, none of them got recorded as sold by BeanCounter. Other items are getting recorded correctly. My suspicion is that selling multiple items with the same base but different suffixes confuses BCMail, but I can't see how in the code :(
    Has anyone else seen this?
  • From testing on WoD Beta, looks like the changing uniqueIDs in mail bug is still present.
  • Nice work, brykrys!
  • Did you try the changes from the SVN (5479-5480)? How are they working for you?

    My PC died last week and only just got it back, so haven't been able to test them thoroughly :(
  • I'm not sure. I just received an item in the mail that didn't seem to get tracked by bean counter. I would have to investigate to see why.
  • Only looked briefly, but I think it's because of the bug in PreGetInboxTextHook that I noticed.

    Looking at the function, it looks like the old code... i.e.:
    function private.PreGetInboxTextHook(n, ...)
    if n and n > 0 then
    local _, _, sender, subject, money, _, daysLeft, _, wasRead, _, _, _ = GetInboxHeaderInfo(n)
    if sender and subject and not wasRead then
    --print("they read", n, sender, subject)
    private.mailReadOveride[n] = sender..n
    elseif wasRead then
    --print("Already read", n, sender, subject)
    end
    end
    return private.GetInboxText(n, ...)
    end

    Did you not patch that one?
  • Hmm.
    I got my updated auctioneer via Curse... but when I look at the code, it looks old. I wonder what happened...
  • Oh... my mistake. I see that what is available via Curse is only r5464 and you committed after that. I'll have to go grab it SVN and try it again.
  • PreGetInboxTextHook got patched in 5467,

    you'd only have the changes if you got them direct from the SVN though.

    Have to get someone to make a new build.
  • I grabbed the latest from trunk... I will let you know how it goes.
Start a New Discussion

Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

In this Discussion

privacy
norganna's addons network · tf2 warehouse · scrap warehouse · auctioneer addon · gatherer addon · addon forums · rdrct