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

RPC unopened #1727

Merged
merged 12 commits into from Feb 22, 2019

Conversation

@guilhermelawless
Copy link
Contributor

commented Feb 13, 2019

Fixes #1166 .

Wiki update:

Unopened

enable_control required, version 19.0+

Returns the total pending balance for unopened accounts in the local database, starting at account (optional) up to count (optional), sorted by account number. Notes: By default excludes the burn account.

Request:

{  
  "action": "unopened",  
  "account": "xrb_1111111111111111111111111111111111111111111111111111hifc8npp",   
  "count": "1"
}

Response:

{
  "accounts": {
    "xrb_1111111111111111111111111111111111111111111111111111hifc8npp": "207034077034226183413773082289554618448"
  }
}
@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

It would be more efficient to iterate pending table keys & check each account existence

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

So the node is aware of all pending blocks? Could you show me an example of where that is done?

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

https://github.com/nanocurrency/nano-node/blob/master/nano/nano_node/entry.cpp#L721
Something like

auto iterator (node.store.pending_begin (transaction));  
auto end (node.store.pending_end (transaction));
while (iterator != end)
{
	nano::pending_key key (i->first);
	nano::account account (key.account);
	nano::pending_info info (i->second);
	if (node.store.account_exists (transaction, account))
	{
		// Skip existing accounts
		iterator = node.store.pending_begin (transaction, nano::pending_key (account.number () + 1, 0));
	}
	else
	{
		auto pending_block_amount (info.amount.number ());
		// ...
		// summ for account
		// ...
		iterator++;
	}
}
@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Yep that's much better.

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@SergiySW i'm getting this weird result on beta, the unopened total is larger than before (notice 34027... vs 34028...).

Supply (available_supply): 340272367920938463463374607431768151531

Total balance with pending (ledger): 232484364820933402871612082323207277732

Total unopened (unopened): 107798002100005060591762525108560933723

Unopened + balance + pending (unopened + ledger): 340282366920938463463374607431768211455

Was matching exactly with the previous approach.

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

@guilhermelawless could you check it with disconnected node?

Show resolved Hide resolved nano/node/rpc.cpp Outdated
Show resolved Hide resolved nano/node/rpc.cpp Outdated
@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@SergiySW i'm getting this weird result on beta, the unopened total is larger than before (notice 34027... vs 34028...).

Supply (available_supply): 340272367920938463463374607431768151531

Total balance with pending (ledger): 232484364820933402871612082323207277732

Total unopened (unopened): 107798002100005060591762525108560933723

Unopened + balance + pending (unopened + ledger): 340282366920938463463374607431768211455

Was matching exactly with the previous approach.

@SergiySW
The mismatching account is the burn: xrb_1111111111111111111111111111111111111111111111111111hifc8npp

In the first method i checked and skipped hash zero so it didn't show up. Would you like to skip it for this RPC or not?

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

This is correct result for total theoretical supply, but incorrect for alvailable_supply ( - burned amount)
Yes, probably better exclude burning account with auto iterator (node.store.pending_begin (transaction, nano::pending_key (1, 0)));

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Ready

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

I guess few things remain we may want.
Similar to "ledger" & other RPC we return list of results with some key i.e.

{
    "accounts": {
       ....
   }
}

And because it's one of the slowest RPC, we may want to limit it with rpc control enabled check

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

left a comment

LGTM after v18 release

@zhyatt zhyatt requested a review from cryptocode Feb 14, 2019

@cryptocode
Copy link
Collaborator

left a comment

LGTM. @guilhermelawless can you prepare markup for the wiki and attach it to this PR?

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

Can do, will take a few days. As a git patch or simply the markdown code?

@SergiySW SergiySW added this to CP0 in V19 Feb 15, 2019

@SergiySW SergiySW self-assigned this Feb 15, 2019

@SergiySW SergiySW requested a review from cryptocode Feb 15, 2019

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

I guess it could be useful to allow specifying starting "account" (default 1) & max "count" similar to ledger

@guilhermelawless guilhermelawless force-pushed the guilhermelawless:rpc/unopened branch from 1ad9769 to 8400913 Feb 19, 2019

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Should be done, wiki entry in the initial comment.

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

It's not that slow anymore with the new approach, so you could consider to leave it without control required.

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2019

How fast is it on live network?
I guess even if it's not slow now with ledger growth (with millions of pending & existing accounts ?) it will become slower & slower

@guilhermelawless

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

You're right. 5 seconds with 27k results, linear scaling means it will indeed become slower.

@SergiySW SergiySW merged commit 36b4654 into nanocurrency:master Feb 22, 2019

2 checks passed

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

@guilhermelawless guilhermelawless deleted the guilhermelawless:rpc/unopened branch Feb 22, 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.