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

Minor adjustments for the tests and codestyle #1066

Merged
merged 1 commit into from
May 7, 2019

Conversation

dsaveliev
Copy link
Member

During the #1062 a couple of minor problems was found:

  • GetVirtualTransactionSize must have a sigOpCost as an argument. Right now this test passes just by coincidence.
  • In p2p_fingerprint.py we need to take unspent coins before the substituted subchain.
  • Clang formatting

Signed-off-by: Dmitry Saveliev [email protected]

@dsaveliev dsaveliev added the refactoring Changes which clean up code but don't change the user-visible behavior label May 6, 2019
@dsaveliev dsaveliev added this to the 0.2 milestone May 6, 2019
@dsaveliev dsaveliev requested review from cmihai, Gnappuraz and a team May 6, 2019 12:38
@dsaveliev dsaveliev self-assigned this May 6, 2019
@cmihai
Copy link
Member

cmihai commented May 6, 2019

GetVirtualTransactionSize must have a sigOpCost as an argument. Right now this test passes just by coincidence.

I think it's not coincidental that the test passes, since the scripts used in the test have a sigop cost of 0 anyway. It doesn't hurt to add the parameter, but Bitcoin doesn't have it, so maybe we should keep everything as it is?

@castarco
Copy link

castarco commented May 6, 2019

Although this PR is quite small, I think we should separate the fix to the p2p_fingerprint test from the style changes.

@dsaveliev
Copy link
Member Author

dsaveliev commented May 6, 2019

I think it's not coincidental that the test passes, since the scripts used in the test have a sigop cost of 0 anyway. It doesn't hurt to add the parameter, but Bitcoin doesn't have it, so maybe we should keep everything as it is?

Not quite, TestMemPoolEntryHelper is initialized with sigOpCost = 4:

spendsCoinbase(false), sigOpCost(4) { }

which goes further to CTxMemPoolEntry
spendsCoinbase, sigOpCost, lp);

which finally affects indexed_transaction_set
mapTx.modify(cit, update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost()));

and finally affects the TX order.
So, here we have a green test just because of fee based on nWeight, which match to the value, calculated inside of indexed_transaction_set:
return (std::max(nWeight, nSigOpCost * nBytesPerSigOp) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR;

Copy link

@castarco castarco left a comment

Choose a reason for hiding this comment

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

utACK b42886c

Copy link
Member

@cmihai cmihai left a comment

Choose a reason for hiding this comment

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

utACK b42886c

@dsaveliev dsaveliev merged commit 6ab049e into dtr-org:master May 7, 2019
@dsaveliev dsaveliev deleted the minor-tests-adjustments branch May 7, 2019 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Changes which clean up code but don't change the user-visible behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants