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

Batch state blocks signatures verification #956

Merged
merged 60 commits into from Oct 14, 2018

Conversation

@SergiySW
Copy link
Collaborator

commented Jul 7, 2018

No description provided.

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2018

Very nice! I'll take a look in a bit, if anyone else can too that'd be good.

messages_pointers[i] = messages[i].bytes.data ();
messages_lengths[i] = sizeof (messages[i].bytes);
public_keys_pointers[i] = public_keys[i].bytes.data ();
signatures_pointers[i] = signatures[i].bytes.data ();

This comment has been minimized.

Copy link
@PlasmaPower

PlasmaPower Jul 11, 2018

Contributor

I'd prefer to prevent an additional copy here. Could we change the function signature to require C style arrays of arrays of bytes?

This comment has been minimized.

Copy link
@SergiySW

SergiySW Jul 11, 2018

Author Collaborator

Sure we can

This comment has been minimized.

Copy link
@PlasmaPower

PlasmaPower Jul 11, 2018

Contributor

While you're at it, where it's called from, instead of copying the e.g. signature bytes we could just make an array of pointers to them.

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

This PR is exciting! I'm worried about messing with signature verification, as that's a very critical piece of code, but this looks good to me.

Does this speed up bootstrapping?

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2018

While it's increasing verification speed up to 40-50% for large amounts of state blocks (I forget to add old test --debug_verify_profile_batch, thx for reminder), currently only 14.4% of blocks are state. So it won't increase current bootstrap speed significantly

@slact

This comment has been minimized.

Copy link

commented Jul 25, 2018

I can confirm a max 40-60% speedup from my batch verification experiments. Increasing the batch size beyond the default (64 i think) doesn't help much either.

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2018

What about this process:

  • In addition to block_processor::blocks there is block_processor::state_blocks
  • block_processor::add puts state blocks in to the state block queue
  • block_processor::process_blocks first runs signature checks on everything in the state_blocks deque. Anything that fails a signature check is dropped. Anything that succeeds is put in to the blocks deque. The blocks deque is then processed as normal
  • Signature checking in removed for state blocks in the ledger_processor
  • A test is added to prove that incorrectly signed state blocks are rejected.
@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2018

Seems valid process

@SergiySW

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2018

Still I think that signature verification shouldn't be removed from ledger processor by default because other functions can use it except block_processor::process_receive_many

Fix
@argakiig

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2018

MSVC conforms to C90 standards and does not support variable length arrays (C99) which is what is causing the appveyor failures

SergiySW added some commits Aug 2, 2018

MSVC workaround
Until variable length arrays support or better VLA emulation

@rkeene rkeene added the major label Oct 12, 2018

@rkeene rkeene requested review from clemahieu and rkeene Oct 12, 2018

{
if (!rai::work_validate (block_a->root (), block_a->block_work ()))
{
std::lock_guard<std::mutex> lock (mutex);
if (blocks_hashes.find (block_a->hash ()) == blocks_hashes.end ())
{
blocks.push_back (std::make_pair (block_a, origination));
blocks_hashes.insert (block_a->hash ());
if (!unchecked && block_a->type () == rai::block_type::state && block_a->link () != node.ledger.epoch_link)

This comment has been minimized.

Copy link
@PlasmaPower

PlasmaPower Oct 12, 2018

Contributor

I believe the unchecked parameter is named incorrectly. Shouldn't it be called checked? If it's true, it bypasses signature validation.

This comment has been minimized.

Copy link
@clemahieu

clemahieu Oct 12, 2018

Collaborator

Can we construct a test on that?

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

Well, probably require renaming. Parameter for state blocks from unchecked table. As all state blocks in unchecked already was validated

This comment has been minimized.

Copy link
@PlasmaPower

PlasmaPower Oct 12, 2018

Contributor

I see. Is the performance improvement worth the risk?

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

I guess yes. Rare risks are signature corruption on disk in unchecked table or bit flips. Same risk for state blocks table & usual signature check.
But this can be moved to separate PR

This comment has been minimized.

Copy link
@PlasmaPower

PlasmaPower Oct 12, 2018

Contributor

In terms of risk I was thinking we might accidentally put unvalidated blocks in the unchecked table.

This comment has been minimized.

Copy link
@PlasmaPower

PlasmaPower Oct 12, 2018

Contributor

Though I think I've brought this up before. Either way should be fine.

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

As far as I see unchecked_put used only with previous_gap or source_gap, that status goes after signature checks for state blocks

This comment has been minimized.

Copy link
@clemahieu

clemahieu Oct 12, 2018

Collaborator

Let’s go with the safer option. Can we make a test that has a block with a bad signature in unchecked and make sure it isn’t processed?

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

Unchecked thing removed. I guess we can return to this after 17.0, now better polish what we have for new release

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

Other than that LGTM

@@ -1440,6 +1460,11 @@ rai::block_hash rai::receive_block::root () const
return hashables.previous;
}

rai::block_hash rai::receive_block::link () const
{
return 0;

This comment has been minimized.

Copy link
@rkeene

rkeene Oct 12, 2018

Contributor

Should we assert(false) here (or just not implement this virtual function for non-state blocks) ? The receive block probably has the equivalent of a link

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

That's similar to source (), previous () & some other functions returning 0 for some block types. We can just not implement such functions when they return 0 I guess to save some lines in code

This comment has been minimized.

Copy link
@clemahieu

clemahieu Oct 12, 2018

Collaborator

Is this a case of using release_assert since we shouldn’t be looking for the link of non-state blocks?

This comment has been minimized.

Copy link
@rkeene

rkeene Oct 12, 2018

Contributor

Either release_assert() or not declaring the virtual methods for these block types which should generate a fatal exception

Show resolved Hide resolved rai/node/node.cpp
@@ -1051,7 +1102,7 @@ void rai::block_processor::process_receive_many (std::unique_lock<std::mutex> &
node.ledger.rollback (transaction, successor->hash ());
}
}
auto process_result (process_receive_one (transaction, block.first, block.second));
auto process_result (process_receive_one (transaction, block.first, block.second, !force));

This comment has been minimized.

Copy link
@rkeene

rkeene Oct 12, 2018

Contributor

Using !force here is not clear that it's directly related to state blocks... can we do something more clear ?

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

Forced state blocks are not validated in verify_state_blocks () function
Because of that we should set set validated_state_block as "false" for forced blocks (!force)

This comment has been minimized.

Copy link
@rkeene

rkeene Oct 12, 2018

Contributor

But !force will evaluate to true when force is false which can happen for things other than state blocks (or verified blocks ?)

This comment has been minimized.

Copy link
@SergiySW

SergiySW Oct 12, 2018

Author Collaborator

Yes, but only state blocks vefirication can be skipped by flag in ledger.processor

SergiySW added some commits Oct 12, 2018

@rkeene

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2018

Awesome work !

@rkeene

rkeene approved these changes Oct 14, 2018

Copy link
Contributor

left a comment

Thanks !

@rkeene rkeene merged commit f438f46 into nanocurrency:master Oct 14, 2018

2 checks passed

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

SergiySW added a commit to SergiySW/raiblocks that referenced this pull request Jan 8, 2019

zhyatt added a commit that referenced this pull request Jan 10, 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.