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.

Sign up for GitHub

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

Jump to bottom

fix: Various fixes for GetProjectedMNPayees #5638

Closed
wants to merge 6 commits into from
Closed

fix: Various fixes for GetProjectedMNPayees #5638

wants to merge 6 commits into from

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Oct 21, 2023

Issue being fixed or feature implemented

Another attempt to fix a crash mentioned in #5590. It's still a workaround and not a fix for the root of the issue (which is still unknown). Also, a couple of additional fixes and optimizations.

What was done?

240e8a9 - a more robust (imo) alternative to the solution provided in #5590
f49dc1e - an additional fix on qt part
3329fcf and b9bc8f2 - small optimizations

How Has This Been Tested?

run local qt node, run tests

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 (for repository code-owners and collaborators only)

@UdjinM6
fix: GetProjectedMNPayees should take care of the block index it's us…
240e8a9 
…ing to check mn_rr internally

This ensures consitensy between pindex and the height of the mn list itself.

Also try to guess mn_rr activation status at nHeight if possible
@UdjinM6
fix: handle GetProjectedMNPayees's failures in `MasternodeList::updat…
f49dc1e 
…eDIP3List()`
@UdjinM6
fix: resulting vector in GetProjectedMNPayees grows to the weighted c…
3329fcf 
…ount, should reserve memory accordingly
@UdjinM6
fix: avoid useless loop in GetProjectedMNPayees after mn_rr activation
b9bc8f2
@UdjinM6 UdjinM6 added this to the 20 milestone Oct 21, 2023
ogabrielides
ogabrielides previously approved these changes Oct 23, 2023
View reviewed changes
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

src/evo/deterministicmns.cpp Outdated Show resolved Hide resolved
@UdjinM6 @ogabrielides
typo
4699be4 
Co-authored-by: Odysseas Gabrielides <odysseas.gabrielides@gmail.com>
ogabrielides
ogabrielides previously approved these changes Oct 23, 2023
View reviewed changes
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.

re-ACK

knst
knst reviewed Oct 26, 2023
View reviewed changes
Copy link
Collaborator

@knst knst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like these 3 commits:

f49dc1e - an additional fix on qt part
3329fcf and b9bc8f2 - small optimizations

Beside me (who met this issue first probably) it seems as similar issue happened by xckd also with dash-qt and it seems as it happens when mnList generated before chain re-organization and GetProjectedMNPayees are requested after by GUI and only GUI (because it uses Tip() instead known pindex).

Taking in consideration that pindex is used here only to determine flag "mn_rr" is activated to see different version of projected mn payees; I propose to take a fix from 5590 as it is; add 3 other commits with improvements and in future after MN_RR would be buried (and height is fixed) we can use hardened height; the issue would be resolved by disappearing.

src/evo/deterministicmns.cpp Outdated
{
if (nCount < 0 ) {
return {};
}

bool isMNRewardReallocation{false};
if (auto pindex = ::ChainActive()[nHeight]; pindex == nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like using here global variable ::ChainActive().Tip().
Also there can be 2 different chain (2 forks on same time) and extracting pindex by [nHeight] can be incorrect value. if after mnList is generated the chain would be switched to other one; the value of ::ChainActive()[nHeight] would be not expected and list would be wrong.

For example call below:

auto projection = deterministicMNManager->GetListForBlock(pindexTip).GetProjectedMNPayees(20);

pindexTip is known but we lost this information.

btw, GetProjecctedMNPayees is just estimation list; is not used in consensus rules; so, I don't think that we need to consider case of LOCKED_IN to make calculation 100% exact for case that happens only once in chain - during MN_RR activation, I vote for simplicity here.

Copy link
Collaborator

@knst knst Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

also, if we will change exact known pindex by using ::ChainActive()[nHeight] it may cause similar bug not only in GUI code but in all usages of GetProjecctedMNPayees (that are non-GUI only), so in this case it can cause this issue getting bigger rather than beeing solved and we won't see it anymore because no crash -> seems works (but it does not). So, now I think that call g_chainman.m_blockman.LookupBlockIndex(blockHash)) is a proper solution until the BIP9 fork is buried (and we can use hardened height and drop g_chainman usage from here)

@UdjinM6
Copy link
Author

UdjinM6 commented Oct 26, 2023

closing in fav of #5590

@UdjinM6 UdjinM6 closed this Oct 26, 2023
knst pushed a commit to knst/dash that referenced this pull request Jan 11, 2024
@knst
Merge bitcoin#20941: rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN e…
b9b22fb 
…xception

74d23bf rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)

Pull request description:

  It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.

  Closes dashpay#5638

ACKs for top commit:
  jonatack:
    ACK 74d23bf

Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
knst pushed a commit to knst/dash that referenced this pull request Jan 12, 2024
@knst
Merge bitcoin#20941: rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN e…
6de63a0 
…xception

74d23bf rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)

Pull request description:

  It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.

  Closes dashpay#5638

ACKs for top commit:
  jonatack:
    ACK 74d23bf

Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
knst pushed a commit to knst/dash that referenced this pull request Jan 13, 2024
@knst
Merge bitcoin#20941: rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN e…
a27dae3 
…xception

74d23bf rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)

Pull request description:

  It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.

  Closes dashpay#5638

ACKs for top commit:
  jonatack:
    ACK 74d23bf

Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
knst pushed a commit to knst/dash that referenced this pull request Jan 13, 2024
@knst
Merge bitcoin#20941: rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN e…
e9d2f99 
…xception

74d23bf rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)

Pull request description:

  It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.

  Closes dashpay#5638

ACKs for top commit:
  jonatack:
    ACK 74d23bf

Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
knst pushed a commit to knst/dash that referenced this pull request Jan 14, 2024
@knst
Merge bitcoin#20941: rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN e…
680d699 
…xception

74d23bf rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)

Pull request description:

  It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.

  Closes dashpay#5638

ACKs for top commit:
  jonatack:
    ACK 74d23bf

Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
PastaPastaPasta pushed a commit to knst/dash that referenced this pull request Jan 16, 2024
@PastaPastaPasta
Merge bitcoin#20941: rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN e…
03d7434 
…xception

74d23bf rpc: document RPC_TRANSACTION_ALREADY_IN_CHAIN exception (Jarol Rodriguez)

Pull request description:

  It is not documented in the `RPCHelpMan` of `sendrawtransaction` that if you attempt to send a transaction which already exists in a block, an `RPC_TRANSACTION_ALREADY_IN_CHAIN` exception will be raised. It is best to make developers aware of this so that it can be properly caught and avoid any headaches.

  Closes dashpay#5638

ACKs for top commit:
  jonatack:
    ACK 74d23bf

Tree-SHA512: d1d5fc242574377c8a76b4ef7b12239996424d8bee186533b5a8fe337bbeb3186e51dbdd28c5eafb982601e44e17b68a7f52db5dd7bc647429f6f95e2de289f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knst knst knst left review comments

@ogabrielides ogabrielides ogabrielides left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
20
Development

Successfully merging this pull request may close these issues.
3 participants
@UdjinM6 @ogabrielides @knst