Skip to content

Conversation

@aguycalled
Copy link
Member

This PR introduces the use of the CoinsDB for the Community Fund, increasing the overall performance and preventing some consistency issues.

@aguycalled
Copy link
Member Author

aguycalled commented May 17, 2019

Things that can be tested:

  • Sync speed/memory usage compared to old system
  • Migration from old DBs
  • Confirm CFund tests pass and all the scenarios are covered

@mxaddict
Copy link
Contributor

@aguycalled I'll be testing the upgrade scenarios in a bit, but doing a testnet/devnet fresh genesis block + staking seems to be working

I will also be testing proposals and payment requests on devnet/testnet

As for the older wallets, what kind of issue might happen if we say upgrade bootstrap server (To use this new unified DB) and an older client bootstraps from that?

@aguycalled
Copy link
Member Author

Lets move that conversation here #488

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.

utACK, looked at the changes, but have not tested locally yet.

@mxaddict
Copy link
Contributor

Compiled, now testing with various datadir scenarios

@mxaddict
Copy link
Contributor

Tested with bootstrap and worked fine, add proposals and payment requests over to coinsdb and removed from the pblocktree successfully locally on Ubuntu 18.04

As for the sanity checks you wanted @aguycalled, don't you think these are fine already?

https://github.com/aguycalled/navcoin-core/blob/ab5fce9af6a63ff1a3b30f34e3df6ff75f3cb717/src/init.cpp#L1654-L1694

@aguycalled
Copy link
Member Author

i've also tried randomly killing the process and restarting checking the state was correctly preserved/reconstructed

@mxaddict
Copy link
Contributor

mxaddict commented May 19, 2019

i've also tried randomly killing the process and restarting checking the state was correctly preserved/reconstructed

I think this would not be an issue, since it does not remove from the cfund copy until it's done adding.

So if it dies in middle of adding, it would just reattempt adding the next init (And from what I can see, it will filter out dups anyway)

So I think that's not an issue.

@mxaddict
Copy link
Contributor

Can confirm that test suite passes on my local build (Ubuntu 18.04)

@mxaddict
Copy link
Contributor

#!/bin/bash

export TIME1=$[ ( $RANDOM % 10 )  + 1 ]s
echo "SLEEPING $TIME1"
sleep $TIME1
kill -9 `cat ~/.navcoin4/testnet3/navcoin.pid`
export TIME1=$[ ( $RANDOM % 10 )  + 1 ]s
echo "SLEEPING $TIME1"
sleep $TIME1
~/navcoind -testnet

I'm running this in a crontab now to make sure that issue #481 no longer occurs

@mxaddict
Copy link
Contributor

@aguycalled were you able to see anything in the log file I sent reagarding this PR?

@aguycalled
Copy link
Member Author

yes, it's probably related.

2019-05-20 21:40:59 ERROR: CheckBlock() : coinbase strdzeel refers wrong payment request hash.
2019-05-20 21:40:59 InvalidChainFound: invalid block=020e0d5e2728926fd45020818fb7f0318d450a81e09636faa308fc4d8ffc193c  height=3145011  log2_work=72.969775  date=2019-05-20 11:35:28

@mxaddict
Copy link
Contributor

yes, it's probably related.

2019-05-20 21:40:59 ERROR: CheckBlock() : coinbase strdzeel refers wrong payment request hash.
2019-05-20 21:40:59 InvalidChainFound: invalid block=020e0d5e2728926fd45020818fb7f0318d450a81e09636faa308fc4d8ffc193c  height=3145011  log2_work=72.969775  date=2019-05-20 11:35:28

Does this need additional code changes? Or should I continue review of current PR?

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.

Tested this thoroughly on Ubuntu 18.04

Only issue I could get to break was force killing the navcoind process repeatedly (Every 20-30 minutes) until it forked.

I think these changes are good to go.

@aguycalled
Copy link
Member Author

yes, it's probably related.

2019-05-20 21:40:59 ERROR: CheckBlock() : coinbase strdzeel refers wrong payment request hash.
2019-05-20 21:40:59 InvalidChainFound: invalid block=020e0d5e2728926fd45020818fb7f0318d450a81e09636faa308fc4d8ffc193c  height=3145011  log2_work=72.969775  date=2019-05-20 11:35:28

Does this need additional code changes? Or should I continue review of current PR?

Tested this thoroughly on Ubuntu 18.04

Only issue I could get to break was force killing the navcoind process repeatedly (Every 20-30 minutes) until it forked.

I think these changes are good to go.

Does this mean you were able to reproduce the issue?

@mxaddict
Copy link
Contributor

No, it only happened once, and I had the node running until now, still fine.

@mxaddict
Copy link
Contributor

I mean I had it running with the CRON that killed it randomly.

@proletesseract
Copy link
Member

compiled using gitian docker on OSX. I have some weird graphics bug where the buttons are appearing pink. I will post a screen shot once the wallet opens up again. I am running the --reindex flag and will see if everything works.

Does running --reindex force the migration to occur? or how does this work?

@mxaddict
Copy link
Contributor

mxaddict commented May 29, 2019 via email

@proletesseract
Copy link
Member

--reindex flag worked. the whole chain loaded with no problems.

@proletesseract
Copy link
Member

seems like you guys have pretty thoroughly looked at this already. Is there anything specific i should be testing on this?

@mxaddict
Copy link
Contributor

@aguycalled @proletesseract I've had a node with this PR's code running for a good week now, still working fine and also staking.

@mxaddict
Copy link
Contributor

@aguycalled, maybe update this branch with latest commits from master branch?

Or rebase 👍

@proletesseract
Copy link
Member

im going to compile in linux and run the test suite then ill be happy to approve/merge once travis works again

@proletesseract
Copy link
Member

restarting travis to see if it passes and I'll merge this if it does. I can build on ubuntu and the rpc-tests all pass. I can build with gitian and the OSX build was able to reindex correctly and function nominally.

@proletesseract proletesseract merged commit a8f425b into navcoin:master May 30, 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