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

fix(miner): ignore lastWork when selecting the best mining candidate #12690

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

Kubuxu
Copy link
Contributor

@Kubuxu Kubuxu commented Nov 12, 2024

Orginal PR #12659
This PR extracts only miner changes as #12659 also makes changes in sync which require more
development and testing.

Previously, we only took the new head if it's heavier than the last
head. Unfortunately, this meant that F3 finalization wasn't properly
propagated to the miner.

In terms of impact:

1. It seems likely that this check was simply defensive as, prior to F3,
the new head should never have a lower weight (unless you're talking to
multiple lotus nodes, I guess...).
2. The `lastWork` field is mostly used to track null blocks. Worst-case
scenario, if we switch heads, we'll attempt to re-mine previous heights.
However, that should be relatively fast and, due to the slash filter, we
won't attempt to re-broadcast any of those blocks.

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
We also perform this check inside `SyncSubmitBlock` so we did have an
effective filter, but this was still wrong.

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
@rjan90 rjan90 requested a review from masih November 12, 2024 11:22
@masih masih merged commit 3d0112e into master Nov 12, 2024
83 of 84 checks passed
@masih masih deleted the kubuxu/fix/last-work branch November 12, 2024 11:29
@rvagg
Copy link
Member

rvagg commented Nov 19, 2024

I think this may have introduced a shutdown race that's caused at least one flaky test in CI: https://github.com/filecoin-project/lotus/actions/runs/11851664737/job/33028560392#step:9:1189

failed to create block:
    github.com/filecoin-project/lotus/miner.(*Miner).mineOne
        /home/runner/work/lotus/lotus/miner/miner.go:634
  - failed to process messages from block template:
    github.com/filecoin-project/lotus/chain/consensus/filcns.(*FilecoinEC).CreateBlock
        /home/runner/work/lotus/lotus/chain/consensus/filcns/mine.go:32
  - building bls amt:
    github.com/filecoin-project/lotus/chain/consensus.CreateBlockHeader
        /home/runner/work/lotus/lotus/chain/consensus/common.go:411
  - badger blockstore closed

I can't come up with a theory about why that might be new with this change because I can imagine how it could happen with the older code (not checking m.stop in at least one more place).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

4 participants