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 for Payment Request reorganizations #456

Merged

Conversation

aguycalled
Copy link
Member

This PR prevents payment requests with invalid hashes (not set yet or out of the main chain) to count for the already requested balance of a proposal.

@mxaddict
Copy link
Contributor

@aguycalled I see that your travis-ci run for this PR on x86_64 linux is failing with similar error to my test, any ideas on what the cause is?

@aguycalled aguycalled requested a review from mxaddict May 1, 2019 09:44
Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

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

I'm not really familiar with the logic in the wallet CFUND code, so I don't think I'm qualified to review that.

But the changes i the unit test seem good.

I just need you to make 1 edit

@@ -3246,7 +3246,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
} else if (out.scriptPubKey.IsPayToPublicKey() || out.scriptPubKey.IsColdStaking()) {
uint160 hashBytes;
int type = 0;
CTxDestination destination;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add .editorconfig to the project?

Copy link
Contributor

Choose a reason for hiding this comment

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

So we can standardize the white space and some other basic configs

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

@aguycalled aguycalled changed the title Ignore payment requests with invalid hashes Fix for Payment Request reorganizations May 1, 2019
@aguycalled
Copy link
Member Author

The issue this PR fixes can be reproduced as follows:

NODE1> staking false
NODE2> staking false
NODE2> addnode 127.0.0.1:node1port onetry
NODE1> generate 300
NODE1> donatefund 100
NODE1> getnewaddress
Save output in %1
NODE2> getnewaddress
Save output in %2
NODE1> sendtoaddress %2 5000000
NODE1> createproposal %1 100 3600 test
Save hash in %3
NODE1> generate 1
NODE2> listproposals
Assert output.size > 0
NODE1> proposalvote %3 yes
NODE1> generate 250
NODE1> createpaymentrequest %3 100 test true
Save output in %4
NODE2> disconnectnode 127.0.0.1:node1port
NODE1> forcetransactions [“%4”]
NODE2> forcetransactions [“%4”]
NODE1> staking true
Wait for 3+ blocks.
Store best block hash in %5
NODE1> staking false
NODE2> staking true
Wait for 1 block. Node 2 only has one output so it will only stake once.
NODE2> staking false
NODE2> addnode 127.0.0.1:node1port onetry
Check that Node2.bestblockhash == %5

Tested succesfully with 2 nodes locally in devnet.
RPC test unit needs to be added.

Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

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

Test looks good.

@proletesseract
Copy link
Member

compiling and starting to review this now.

@mxaddict
Copy link
Contributor

mxaddict commented May 1, 2019

compiling and starting to review this now.

Looks like the latest build has an issue with cfund-paymentrequest-state-reorg.py

@mxaddict
Copy link
Contributor

mxaddict commented May 2, 2019 via email

@proletesseract
Copy link
Member

proletesseract commented May 2, 2019

Roger that.

You mean ACK :P

@mxaddict
Copy link
Contributor

mxaddict commented May 2, 2019

Roger that.

You mean ACK :P

I see ACK on alot of opensource projects, what does it actually stand for? :)

@proletesseract
Copy link
Member

proletesseract commented May 2, 2019

Roger that.

You mean ACK :P

I see ACK on alot of opensource projects, what does it actually stand for? :)

It's just short for "Acknowledged" AFAIK.

see: bitcoin/bitcoin#6100

@aguycalled
Copy link
Member Author

I've changed the check so it looks at chainwork instead of nbits.
chainwork = prevblock.chainwork + nbits

@mxaddict
Copy link
Contributor

mxaddict commented May 2, 2019

Looks like final commit fixed the issue with the test.

@craigmacgregor ready to merge? :)

@proletesseract
Copy link
Member

Travis failing again. Investigating.

@aguycalled
Copy link
Member Author

it's failing again uniquely because of a random fee in the coldstaking_* tests

@aguycalled aguycalled requested a review from mxaddict May 2, 2019 20:54
@proletesseract
Copy link
Member

proletesseract commented May 2, 2019

my local build and tests pass on Ubuntu 18.04, waiting for Travis.

@proletesseract proletesseract merged commit 688bf4d into navcoin:master May 2, 2019
proletesseract added a commit to proletesseract/navcoin-core that referenced this pull request May 2, 2019
proletesseract added a commit to proletesseract/navcoin-core that referenced this pull request May 2, 2019
@proletesseract proletesseract mentioned this pull request May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants