Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Unexpected Unassign #654

Closed
0x4007 opened this issue Aug 22, 2023 · 13 comments · Fixed by #660
Closed

Unexpected Unassign #654

0x4007 opened this issue Aug 22, 2023 · 13 comments · Fixed by #660

Comments

@0x4007
Copy link
Member

0x4007 commented Aug 22, 2023

This should be diagnosed and fixed. This should not have happened. Also for auditing/scoring this is bad for the bounty hunter to be unassigned because now they technically would not have gotten credit (for XP) for this closed issue (they are no longer assigned.)

Fortunately, at least, the permit was generated probably due to a race condition.

@whilefoo - Releasing the bounty back to dev pool because the allocated duration already ended!
Last activity time: Tue Jul 25 2023 08:45:17 GMT+0000 (Coordinated Universal Time)

Originally posted by @ubiquibot[bot] in #496 (comment)

@0x4007 0x4007 changed the title Unexpected Release Unexpected Unassign Aug 22, 2023
@gitcoindev
Copy link
Contributor

Hi @pavlovcik I had a look at sources out of curiosity how the state machine works.
Do we have access to the bot logs?

I guess the root cause was that wildcardProcessors kicked in on a GitHub event other than GithubEvent.PUSH when you added the pull request to the merge queue.

@[pavlovcik](https://github.com/pavlovcik) pavlovcik added this pull request to the [merge queue](https://github.com/ubiquity/ubiquibot/queue/development) [5 hours ago](https://github.com/ubiquity/ubiquibot/pull/545#event-10159715516)
Merged via the queue into ubiquity:development with commit [12777f0](https://github.com/ubiquity/ubiquibot/commit/12777f051063eee4f244df33cf357cb3b576252a) [5 hours ago](https://github.com/ubiquity/ubiquibot/pull/545#event-10159718916)

I further assume that this bounty must have had passedDuration >= disqualifyTime and unassign was executed here
https://github.com/ubiquity/ubiquibot/blob/development/src/handlers/wildcard/unassign.ts#L54

Normally the 'GithubEvent.PUSH' event is received when the PR is merged is processed here https://github.com/ubiquity/ubiquibot/blob/development/src/handlers/processors.ts#L68

And I suspect that when you added the PR to the merge queue, it triggered another event than GithubEvent.PUSH (merge_group event), this triggered the wildcardProcessors first, which unexpectedly unassigned @whilefoo .

So the way to fix this would be to add the same processor for the GithubEvent.PUSH and the event triggered after adding a pull request to the merge queue (https://docs.github.com/en/webhooks-and-events/webhooks/webhook-events-and-payloads#merge_group). I would confirm this and submit fix if I had access to logs.

@0x4007
Copy link
Member Author

0x4007 commented Aug 22, 2023

Thanks for sharing your hypothesis! @devpanther is working on the real-time log app right now over at:

Since it is not completed, you might be able to use the latest code there and view the logs to complete the current task.

The logging app basically just reads directly from our database with a Supabase anonymous key (read only access.)

@gitcoindev
Copy link
Contributor

ok great, I will look into this and try to fix tomorrow.

@gitcoindev
Copy link
Contributor

/start

@ubiquibot
Copy link

ubiquibot bot commented Aug 23, 2023

Deadline Thu, 24 Aug 2023 05:34:17 UTC
Registered Wallet 0x7e92476D69Ff1377a8b45176b1829C4A5566653a
Tips:
  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address @user.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the bounty.

    @gitcoindev
    Copy link
    Contributor

    I went an extra mile here with the setup, was able to reproduce and fix the issue. The problem is not the merge_group event , but the fact that when merge queues are used, GitHub sends three! push events instead of one. This is caused by the way merge queues work:

    https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#how-merge-queues-work

    As pull requests are added to the merge queue, the merge queue ensures that they are merged in a first-in-first-out order where the required checks are always satisfied.

    A merge queue creates temporary branches with a special prefix to validate pull request changes. When a pull request is added to the merge queue, the changes in the pull request are grouped into a merge_group with the latest version of the base_branch as well as changes from pull requests ahead of it in the queue. GitHub will merge all these changes into the base_branch once the checks required by the branch protections of base_branch pass.

    Therefore, behind the scenes , when using merge queues, GitHub creates a special temporary branch, pushes pull requests into the temporary branches and then pushes them back into a base branch using a queue mechanism.

    I modified my local bot sources to log full webhook payloads to console, modified DISQUALIFY_TIME="2 minutes" and modified labels to process 1 Hour as 1 minute to faster reproduce this bug.

    evm-network-id: 100
    price-multiplier: 1.5
    issue-creator-multiplier: 2
    time-labels:
      - name: "Time: <1 Hour"
        weight: 0.125
        value: 60
      - name: "Time: <2 Hours"
        weight: 0.25
        value: 120
    ...
    

    I had to create my GitHub organization as normal accounts do not support merge queues, where I installed my modified bot
    I was able to reproduce the bug here:

    korrrba/test-ubiquibot#9

    and was able to QA my fix here:

    korrrba/test-ubiquibot#11

    The solution is not to trigger the wildcard handlers during GitHub PUSH event, just the dedicated webhook push handler should be triggered then. The wildcard handlers are triggered anyway shortly after when bot comments and when it closes the issue.

    I will submit a pull request with a fix asap.

    gitcoindev added a commit to gitcoindev/ubiquibot that referenced this issue Aug 23, 2023
    @0x4007
    Copy link
    Member Author

    0x4007 commented Aug 23, 2023

    @gitcoindev lovely research. Thanks a ton for your contributions.

    @ubiquibot
    Copy link

    ubiquibot bot commented Aug 23, 2023

    [ CLAIM 300 WXDAI ]

    0x7e92476D...A5566653a

    If you've enjoyed your experience in the DevPool, we'd appreciate your support. Follow Ubiquity on GitHub and star this repo. Your endorsement means the world to us and helps us grow!
    We are excited to announce that the DevPool and UbiquiBot are now available to partners! Our ideal collaborators are globally distributed crypto-native organizations, who actively work on open source on GitHub, and excel in research & development. If you can introduce us to the repository maintainers in these types of companies, we have a special bonus in store for you!

    @ubiquibot
    Copy link

    ubiquibot bot commented Aug 23, 2023

    pavlovcik: [ CLAIM 0 WXDAI ]

    @0x4007
    Copy link
    Member Author

    0x4007 commented Aug 23, 2023

    /query @pavlovcik

    @ubiquibot
    Copy link

    ubiquibot bot commented Aug 23, 2023

    @pavlovcik's wallet address is 0x4007CE2083c7F3E18097aeB3A39bb8eC149a341d and multiplier is 1

    @gitcoindev
    Copy link
    Contributor

    Hmm strange, why 0 WXDAI for you? perhaps an another bug?

    @0x4007
    Copy link
    Member Author

    0x4007 commented Aug 23, 2023

    Hmm strange, why 0 WXDAI for you? perhaps an another bug?

    This was fixed in a later commit (which currently exists on the development branch) to not display rewards of 0, and also to explicitly state that it is for comment incentives. It looks like it rounded down to the nearest dollar (0)

    We will roll out the new update within the next few days!

    Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
    Projects
    None yet
    Development

    Successfully merging a pull request may close this issue.

    2 participants