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

Add confirmation height #1770

Merged
merged 30 commits into from Mar 3, 2019

Conversation

@wezrule
Copy link
Collaborator

commented Feb 23, 2019

These are summarized notes given before the start of this work:

When we confirm a block what we want to set the account's confirmation height to be equal to the height of the block just confirmed. So if the confirmation height was 2 and the block height is 7, we set the confirmation height to 7. For each block that was implicitly confirmed; 7-3, we check if it’s a receive block. If it’s a receive block we update the confirmation height of the sender in the same fashion. The algorithm needs to be iterative, not recursive since this could be a deep confirmation process. When we update the database, all confirmation heights start as zero and when the first confirmed block comes off the live network it’s going to confirm a ton of accounts.

The sideband upgrade has been made synchronous (approved by @clemahieu) to make the process simpler for future upgrades. This means nodes will be very slow to start if going from v17.1 -> v19 however the big services/exchanges should have been made aware of this during the v18 release announcement so it should not affect many nodes.

The block_info New RPC block_confirmed has been added to check if a block has been confirmed and the account_info has been modified to output the confirmation height for the height as well. MSVC only compiles up to 128 if/elseif blocks (well 128 nested blocks) which was reached after I added 2 RPC calls. I have combined the password/payment RPC calls into another one temporarily until a better solution can be found. A possible solution is to use a std::map<std::string, std::function<void(rpc_handler*)>> which maps action => handler for functions with no arguments.

--snapshot now accepts an optional confirmation_height_clear option for clearing the confirmation heights (setting to 0) for all accounts.
--confirmation_height_clear CLI option can also be used directly as well, with an optional account for clearing single account.

Updated nano/core_test/versioning.cpp to have tests for all account_info_v* classes

lmdb.cpp - nano::mdb_store::account_get() didn't check the error condition from info_a.deserialize (stream); and same in pending_get(), so I have added those.

There are still some questions around pending blocks, but this can always be done in a separate PR.

@wezrule wezrule added the tool label Feb 26, 2019

@wezrule wezrule force-pushed the wezrule:confirmation_height branch 2 times, most recently from ef737c6 to c5be171 Feb 28, 2019

@zhyatt zhyatt added this to CP1 in V19 Feb 28, 2019

@argakiig
Copy link
Collaborator

left a comment

Formatting and LGTM

Show resolved Hide resolved nano/node/cli.cpp Outdated
@SergiySW
Copy link
Collaborator

left a comment

LGTM (squash)

@wezrule wezrule requested a review from cryptocode Mar 1, 2019

Show resolved Hide resolved nano/node/node.cpp Outdated
@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2019

Stein has ran it for 12 hours and found most blocks spend < 1ms inside the iterative function add_confirmation_heights with a few outliers when there are long chains but they don't lock long enough to cause rpc lockups. Merging this in now so that any problems can be found sooner. Zach has some further questions about functionality but these can be investigated separately as this is a building block for further enhancements.

@wezrule wezrule merged commit 2a3b1d5 into nanocurrency:master Mar 3, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wezrule wezrule deleted the wezrule:confirmation_height branch Mar 3, 2019

@zhyatt zhyatt added the major label Apr 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.