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: multiple minor errors for Dash Core pre v22 #6373

Merged
merged 8 commits into from
Oct 29, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Oct 29, 2024

Issue being fixed or feature implemented

It fixes several minor issues:

  • error messages in logs on regtest:

    [httpworker.0] [masternode/payments.cpp:116] [IsTransactionValid] CMNPaymentsProcessor::IsTransactionValid -- ERROR! Failed to get payees for block at height 110
    [httpworker.0] [masternode/payments.cpp:278] [IsBlockPayeeValid] CMNPaymentsProcessor::IsBlockPayeeValid -- Valid masternode payment at height 110: CTransaction(hash=837e35fab5, ver=3, type=5, vin.size=1, vout.size=1, nLockTime=0, vExtraPayload.size=70)
           CTxIn(COutPoint(0000000000000000000000000000000000000000000000000000000000000000, 4294967295), coinbase 016e0101)
           CTxOut(nValue=500.00000000, scriptPubKey=76a9148708dff2bf8b31363cb4201d)
    
  • error messages for non-existing files for first run:

     [      init] [util/system.h:57] [error] ERROR: CoreRead: Failed to open file DASH/node0/regtest/governance.dat
    
  • non-dashified bitcoin-cli in doc/JSON-RPC-interface.md

  • incorrect initialization of CMerkleBlock in functional tests which can cause an un-explainable failure

  • use random ports for interface_zmq_dash.py to let tests run simultaneously

  • fix missing todo about using sethdseed after backport: bitcoin#18836, #19046, #19490, #20403, #21127, #21238, #21329 - descriptor wallets part V & sethdseed #6017

  • optimization of local run of functional tests (slowest go first)

What was done?

See each commit

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • 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)

@knst knst changed the title fix: multiple minor for Dash Core pre v22 fix: multiple minor errors for Dash Core pre v22 Oct 29, 2024
@knst knst marked this pull request as ready for review October 29, 2024 17:03
@knst knst mentioned this pull request Oct 29, 2024
5 tasks
… get payees for block at height"

    [httpworker.0] [masternode/payments.cpp:116] [IsTransactionValid] CMNPaymentsProcessor::IsTransactionValid -- ERROR! Failed to get payees for block at height 110
    [httpworker.0] [masternode/payments.cpp:278] [IsBlockPayeeValid] CMNPaymentsProcessor::IsBlockPayeeValid -- Valid masternode payment at height 110: CTransaction(hash=837e35fab5, ver=3, type=5, vin.size=1, vout.size=1, nLockTime=0, vExtraPayload.size=70)
                                           CTxIn(COutPoint(0000000000000000000000000000000000000000000000000000000000000000, 4294967295), coinbase 016e0101)
                                           CTxOut(nValue=500.00000000, scriptPubKey=76a9148708dff2bf8b31363cb4201d)
…ssage in logs

exmple of error during first start of dashd:
    [      init] [util/system.h:57] [error] ERROR: CoreRead: Failed to open file DASH/node0/regtest/governance.dat
On CI it is not critical, because there are only 4-8 parallel jobs,
while locally it can be 12-20 or even more
@UdjinM6 UdjinM6 added this to the 22 milestone Oct 29, 2024
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 2026c59

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

LGTM, small suggestion

@@ -200,10 +200,10 @@ class CFlatDB
int64_t nStart = GetTimeMillis();

LogPrintf("Writing info to %s...\n", strFilename);
CoreWrite(objToSave);
const bool ret = CoreWrite(objToSave);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably not print "dump finished" if ret is false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that log is for benchmark purposes, but I am ready to apply suggestions

@@ -51,8 +51,12 @@ CAmount PlatformShare(const CAmount reward)
LogPrint(BCLog::MNPAYMENTS, "CMNPaymentsProcessor::%s -- MN reward %lld reallocated to credit pool\n", __func__, platformReward);
voutMasternodePaymentsRet.emplace_back(platformReward, CScript() << OP_RETURN);
}

auto dmnPayee = m_dmnman.GetListForBlock(pindexPrev).GetMNPayee(pindexPrev);
const auto mnList = m_dmnman.GetListForBlock(pindexPrev);
Copy link
Member

Choose a reason for hiding this comment

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

678db6f: where'd you find this crash?

Copy link
Collaborator Author

@knst knst Oct 29, 2024

Choose a reason for hiding this comment

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

it's not a crash and even not a failure. It is not expecting ERROR strings in logs on Regtest for every block since DIP0003 is activated but no masternodes created.

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.

generally LGTM 2026c59; one question

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 2026c59

@PastaPastaPasta PastaPastaPasta merged commit 5d2fde8 into dashpay:develop Oct 29, 2024
38 of 47 checks passed
@knst knst deleted the multiple-fixes-2024-10-p1 branch October 31, 2024 13:49
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