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: possible assert call if nHeight in CDeterministicMNList is higher then Tip #5590

Merged

Conversation

knst
Copy link
Collaborator

@knst knst commented Sep 28, 2023

Issue being fixed or feature implemented

fix: possible assert call if nHeight in CDeterministicMNListDiff is higher than Tip

Example of new log:

2023-09-28T17:35:50Z GetProjectedMNPayeesAtChainTip WARNING pindex is nullptr due to height=914160 chain height=914159

instead assert call:

...
     #6  0x00007ffff7a33b86 in __assert_fail (assertion=0x55555783afd2 "pindex", file=0x5555577f2ed8 "llmq/utils.cpp", line=730,
            function=0x5555577f2448 "bool llmq::utils::IsMNRewardReallocationActive(const CBlockIndex*)") at ./assert/assert.c:101
     #7  0x0000555555ab7daf in llmq::utils::IsMNRewardReallocationActive (pindex=<optimized out>) at llmq/utils.cpp:730
     #8  0x00005555559458ad in CDeterministicMNList::GetProjectedMNPayees (this=this@entry=0x7fffffffc690, pindex=0x0, nCount=<optimized out>, nCount@entry=2147483647)
            at evo/deterministicmns.cpp:231
     #9  0x000055555594614f in CDeterministicMNList::GetProjectedMNPayeesAtChainTip (this=this@entry=0x7fffffffc690, nCount=nCount@entry=2147483647) at evo/deterministicmns.cpp:216
     #10 0x00005555558c9f51 in MasternodeList::updateDIP3List (this=this@entry=0x55555908cfd0) at qt/masternodelist.cpp:194
     #11 0x00005555558ca9a0 in MasternodeList::updateDIP3ListScheduled (this=0x55555908cfd0) at qt/masternodelist.cpp:157
     #12 0x000055555684a60f in void doActivate<false>(QObject*, int, void**) ()
     #13 0x00005555568525b1 in QTimer::timerEvent(QTimerEvent*) ()
     #14 0x0000555556844ce5 in QObject::event(QEvent*) ()
     #15 0x0000555556ac3252 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
     #16 0x000055555681e6b8 in QCoreApplication::sendEvent(QObject*, QEvent*) ()
     #17 0x000055555686de2a in QTimerInfoList::activateTimers() ()
     #18 0x000055555686be84 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
     #19 0x00005555569bf8a2 in QXcbUnixEventDispatcher::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
     #20 0x000055555681caf6 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
     #21 0x0000555556825f8a in QCoreApplication::exec() ()
...

What was done?

ClientModel returns now a pair: MNList and CBlockIndex; so, we always know the which one has been used even if current chain is switched.

How Has This Been Tested?

Run on my localhost from c034ff0c2606142ba3e8894bc74f693b87374e5c - aborted with backtrace like above.
With both of commit - no assert more.

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst added this to the 20 milestone Sep 28, 2023
@knst knst changed the title fix: possible assert call if nHeight in CDeterministicMNListDiff is higher than Tip fix: possible assert call if nHeight in CDeterministicMNList is higher than Tip Sep 28, 2023
@knst knst changed the title fix: possible assert call if nHeight in CDeterministicMNList is higher than Tip fix: possible assert call if nHeight in CDeterministicMNList is higher then Tip Sep 28, 2023
@knst
Copy link
Collaborator Author

knst commented Sep 28, 2023

@UdjinM6 @PastaPastaPasta please, suggest me which of 2 solution [see 2 commits in PR] to apply. I don't like both of them but any of them is better then current develop over 9000 percents.

Or any alternate better solution?

UPD: done

Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for 04fc666.
Feels more right to me.

@PastaPastaPasta
Copy link
Member

What if we adjust the calling locations to ensure they're not null there?

@knst
Copy link
Collaborator Author

knst commented Sep 29, 2023

What if we adjust the calling locations to ensure they're not null there?

It can be wrong chain (wrong fork) anyway

UdjinM6
UdjinM6 previously approved these changes Oct 3, 2023
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

ogabrielides
ogabrielides previously approved these changes Oct 3, 2023
Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@PastaPastaPasta
Copy link
Member

I think concept NACK, but not positive;

This function is called from these locations, the problematic call is the qt/masternodelist.cpp location;

src/governance/governance.cpp:679:        auto mn_payees = mnList.GetProjectedMNPayeesAtChainTip();
src/qt/masternodelist.cpp:194:    auto projectedPayees = mnList.GetProjectedMNPayeesAtChainTip();
src/rpc/masternode.cpp:139:    auto payees = mnList.GetProjectedMNPayeesAtChainTip(heightShift);

I believe that this issue is only possible when the mnList is stale; I think there is a root problem here that cached qt mnListCached is not properly updated when the block was rejected/marked as conflicted/undone. see src/qt/clientmodel.h:121. I'm not STRICTLY opposed to the fix as done here, but I would like to see the underlying issue fixed or investigated more.

@knst knst modified the milestones: 20, 21 Oct 23, 2023
knst and others added 6 commits October 26, 2023 20:32
…igher than Tip

Example of new log:
```
2023-09-28T17:35:50Z GetProjectedMNPayeesAtChainTip WARNING pindex is nullptr due to height=914160 chain height=914159
```

instead assert call:
```
...
 dashpay#6  0x00007ffff7a33b86 in __assert_fail (assertion=0x55555783afd2 "pindex", file=0x5555577f2ed8 "llmq/utils.cpp", line=730,
        function=0x5555577f2448 "bool llmq::utils::IsMNRewardReallocationActive(const CBlockIndex*)") at ./assert/assert.c:101
 dashpay#7  0x0000555555ab7daf in llmq::utils::IsMNRewardReallocationActive (pindex=<optimized out>) at llmq/utils.cpp:730
 dashpay#8  0x00005555559458ad in CDeterministicMNList::GetProjectedMNPayees (this=this@entry=0x7fffffffc690, pindex=0x0, nCount=<optimized out>, nCount@entry=2147483647)
        at evo/deterministicmns.cpp:231
 dashpay#9  0x000055555594614f in CDeterministicMNList::GetProjectedMNPayeesAtChainTip (this=this@entry=0x7fffffffc690, nCount=nCount@entry=2147483647) at evo/deterministicmns.cpp:216
 dashpay#10 0x00005555558c9f51 in MasternodeList::updateDIP3List (this=this@entry=0x55555908cfd0) at qt/masternodelist.cpp:194
 dashpay#11 0x00005555558ca9a0 in MasternodeList::updateDIP3ListScheduled (this=0x55555908cfd0) at qt/masternodelist.cpp:157
 dashpay#12 0x000055555684a60f in void doActivate<false>(QObject*, int, void**) ()
 dashpay#13 0x00005555568525b1 in QTimer::timerEvent(QTimerEvent*) ()
 dashpay#14 0x0000555556844ce5 in QObject::event(QEvent*) ()
 dashpay#15 0x0000555556ac3252 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
 dashpay#16 0x000055555681e6b8 in QCoreApplication::sendEvent(QObject*, QEvent*) ()
 dashpay#17 0x000055555686de2a in QTimerInfoList::activateTimers() ()
 dashpay#18 0x000055555686be84 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
 dashpay#19 0x00005555569bf8a2 in QXcbUnixEventDispatcher::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
 dashpay#20 0x000055555681caf6 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
 dashpay#21 0x0000555556825f8a in QCoreApplication::exec() ()
...
```
@knst knst dismissed stale reviews from ogabrielides and UdjinM6 via e258c36 October 26, 2023 13:43
@knst knst force-pushed the fix-possible-crash-deterministic branch from 04fc666 to e258c36 Compare October 26, 2023 13:43
@knst knst requested a review from UdjinM6 October 26, 2023 13:46
@knst
Copy link
Collaborator Author

knst commented Oct 26, 2023

the problematic call is the qt/masternodelist.cpp location

The problematic call is indeed qt/masternodelist.cpp location because mnList is saved once and later used (can be much later than it is init).
Other places use mnList right after initialization so chain doesn't have chance to switch usually so it has not been reproduced by non-GUI code yet but also can happen.

latest commits fix all usages of GetProjectedMNPayeesAtChainTip

@UdjinM6 UdjinM6 modified the milestones: 21, 20 Oct 26, 2023
@UdjinM6
Copy link

UdjinM6 commented Oct 26, 2023

fails to compile for me

qt/masternodelist.cpp:232:61: error: reference to local binding 'mnList' declared in enclosing function 'MasternodeList::updateDIP3List'

d109650 helps

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it looks good 👍 (reindexed and invalidated/reconsidered ~100 blocks from the tip).

ACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for squash merge

@PastaPastaPasta PastaPastaPasta merged commit a0c8c9f into dashpay:develop Oct 28, 2023
6 of 8 checks passed
knst added a commit to knst/dash that referenced this pull request Oct 30, 2023
…igher than Tip - cummulative from dashpay#5590

Example of new log:
```
2023-09-28T17:35:50Z GetProjectedMNPayeesAtChainTip WARNING pindex is nullptr due to height=914160 chain height=914159
```

instead assert call:
```
...
 dashpay#6  0x00007ffff7a33b86 in __assert_fail (assertion=0x55555783afd2 "pindex", file=0x5555577f2ed8 "llmq/utils.cpp", line=730,
        function=0x5555577f2448 "bool llmq::utils::IsMNRewardReallocationActive(const CBlockIndex*)") at ./assert/assert.c:101
 dashpay#7  0x0000555555ab7daf in llmq::utils::IsMNRewardReallocationActive (pindex=<optimized out>) at llmq/utils.cpp:730
 dashpay#8  0x00005555559458ad in CDeterministicMNList::GetProjectedMNPayees (this=this@entry=0x7fffffffc690, pindex=0x0, nCount=<optimized out>, nCount@entry=2147483647)
        at evo/deterministicmns.cpp:231
 dashpay#9  0x000055555594614f in CDeterministicMNList::GetProjectedMNPayeesAtChainTip (this=this@entry=0x7fffffffc690, nCount=nCount@entry=2147483647) at evo/deterministicmns.cpp:216
 dashpay#10 0x00005555558c9f51 in MasternodeList::updateDIP3List (this=this@entry=0x55555908cfd0) at qt/masternodelist.cpp:194
 dashpay#11 0x00005555558ca9a0 in MasternodeList::updateDIP3ListScheduled (this=0x55555908cfd0) at qt/masternodelist.cpp:157
 dashpay#12 0x000055555684a60f in void doActivate<false>(QObject*, int, void**) ()
 dashpay#13 0x00005555568525b1 in QTimer::timerEvent(QTimerEvent*) ()
 dashpay#14 0x0000555556844ce5 in QObject::event(QEvent*) ()
 dashpay#15 0x0000555556ac3252 in QApplicationPrivate::notify_helper(QObject*, QEvent*) ()
 dashpay#16 0x000055555681e6b8 in QCoreApplication::sendEvent(QObject*, QEvent*) ()
 dashpay#17 0x000055555686de2a in QTimerInfoList::activateTimers() ()
 dashpay#18 0x000055555686be84 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
 dashpay#19 0x00005555569bf8a2 in QXcbUnixEventDispatcher::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
 dashpay#20 0x000055555681caf6 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) ()
 dashpay#21 0x0000555556825f8a in QCoreApplication::exec() ()
...
```

fix: use g_chainman.m_blockman.LookupBlockIndex for any known block (even wrong tip)

fix: handle GetProjectedMNPayees's failures in `MasternodeList::updateDIP3List()`

fix: resulting vector in GetProjectedMNPayees grows to the weighted count, should reserve memory accordingly

fix: avoid useless loop in GetProjectedMNPayees after mn_rr activation

fix: pass proper CBlockIndex from ClientModel

fix statusItem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants