Add support for hashing SCPStatement & SCPEnvelope #546

AndrejMitrovic · 2020-02-17T14:32:04Z

This will be needed for signing SCP statements.

Edit: Now it's also necessary to fix the issue in #716

I'm not sure where to place this hashing support, other than into the Nominator module because this is the only place it will be used.

I cannot move it to scpd.types.Stellar_SCP because this module should not import hashing routines from agora.*, and I'm not sure agora.common.Hash is a good place for it either.

Also side-note: maybe there should be a generic way of testing hashing behavior for any struct types. Something like a "fuzzer" type helper function that manipulates each field and verifies that the hash changed.

codecov · 2020-02-18T08:30:42Z

Codecov Report

Merging #546 into v0.x.x will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           v0.x.x     #546     +/-   ##
=========================================
+ Coverage   91.14%   91.25%   +0.1%     
=========================================
  Files          63       63             
  Lines        4809     4892     +83     
=========================================
+ Hits         4383     4464     +81     
- Misses        426      428      +2

Flag	Coverage Δ
#integration	`52.68% <0%> (-1.17%)`	⬇️
#unittests	`90.01% <100%> (+0.11%)`	⬆️

Impacted Files	Coverage Δ
source/agora/consensus/protocol/Nominator.d	`91.39% <100%> (+6.93%)`	⬆️
source/agora/node/BlockStorage.d	`69.25% <0%> (-1.12%)`	⬇️
source/agora/common/Hash.d	`98.33% <0%> (+1.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a99b4d...1d20c65. Read the comment docs.

AndrejMitrovic · 2020-04-03T04:18:22Z

Disallowing sources: ubuntu-toolchain-r-test
To add unlisted APT sources, follow instructions in https://docs.travis-ci.com/user/installing-dependencies#Installing-Packages-with-the-APT-Addon
13.81s$ travis_apt_get_update
Installing APT Packages
0.40s$ sudo -E apt-get -yq --no-install-suggests --no-install-recommends $(travis_apt_get_options) install libsodium-dev g++-9 libsqlite3-dev
Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package g++-9
E: Couldn't find any package by regex 'g++-9'
apt-get.diagnostics
apt-get install failed
The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends $(travis_apt_get_options) install libsodium-dev g++-9 libsqlite3-dev" failed and exited with 100 during .

Is Travis broken all of a sudden?

Geod24 · 2020-04-03T06:31:56Z

source/agora/consensus/protocol/Nominator.d

+    static assert(SCPNomination.sizeof == 80);
+
+    /// instance
+    const SCPStatement st;


Doesn't that do a copy tho ?

Geod24 · 2020-04-03T06:34:21Z

source/agora/consensus/protocol/Nominator.d

+
+    ***************************************************************************/
+
+    public void computeHash (const ref SCPStatement._pledges_t._prepare_t prep,


Looks like those could be static ?

Same for the following ones

AndrejMitrovic · 2020-04-03T07:03:45Z

Fixed the comments. And also added hashing for SCPEnvelope, which just embeds the SCPStatement and the signature.

AndrejMitrovic · 2020-04-03T08:02:26Z

Hmm on ldc I am getting different hashes compared to DMD. lol

AndrejMitrovic · 2020-04-03T08:31:19Z

I found a weird bug with lambdas.

This code is OK:

// OK
auto sign_data = pair.secret.sign(msg[]);

() @trusted {
    auto sig = sign_data[];
    env.signature = sig.toVec();
    writefln("\n\nbef: %s", env.signature);
}();

writefln("\n\naft: %s", env.signature);

Here the signature will be the same inside the lambda and outside of it. Output:

bef: [ 93, 231, 92, 250, 9, 186, 244, 162, 189, 114, 110, 68, 95, 241, 177, 218, 40, 166, 154, 157, 8, 1, 223, 5, 78, 116, 107, 134, 111, 93, 240, 85, 182, 29, 241, 59, 88, 47, 230, 118, 206, 240, 76, 117, 70, 192, 24, 156, 38, 225, 143, 125, 152, 217, 191, 189, 111, 165, 217, 8, 212, 240, 55, 9 ]


aft: [ 93, 231, 92, 250, 9, 186, 244, 162, 189, 114, 110, 68, 95, 241, 177, 218, 40, 166, 154, 157, 8, 1, 223, 5, 78, 116, 107, 134, 111, 93, 240, 85, 182, 29, 241, 59, 88, 47, 230, 118, 206, 240, 76, 117, 70, 192, 24, 156, 38, 225, 143, 125, 152, 217, 191, 189, 111, 165, 217, 8, 212, 240, 55, 9 ]

But if I move the sign data inside the lambda:

() @trusted {
    // FAIL
    auto sign_data = pair.secret.sign(msg[]);
    auto sig = sign_data[];
    env.signature = sig.toVec();
    writefln("\n\nbef: %s", env.signature);
}();

writefln("\n\naft: %s", env.signature);

Then the signatures are different:

bef: [ 93, 231, 92, 250, 9, 186, 244, 162, 189, 114, 110, 68, 95, 241, 177, 218, 40, 166, 154, 157, 8, 1, 223 , 5, 78, 116, 107, 134, 111, 93, 240, 85, 182, 29, 241, 59, 88, 47, 230, 118, 206, 240, 76, 117, 70, 192, 24, 156, 38, 225, 143, 125, 152, 217, 191, 189, 111, 165, 217, 8, 212, 240, 55, 9 ]


aft: [ 12, 0, 0, 0, 0, 0, 0, 0, 11, 0, 0, 0, 0, 0, 0, 0, 10, 0, 0, 0, 0, 0, 0, 0, 176, 171, 55, 192, 255, 127 , 0, 0, 224, 186, 88, 74, 49, 127, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 165, 217, 8, 212, 240, 55, 9 ]

I've tried GC.disable() but that didn't do the trick. It almost seems like maybe the memory is allocated on the stack? Hmm..

AndrejMitrovic · 2020-04-03T09:09:40Z

Fixed. Should go green soon.

Geod24

Just 3 nits, otherwise LGTM. Feel free to fix and merge at will.

Geod24 · 2020-04-03T09:15:44Z

source/agora/consensus/protocol/Nominator.d

+    private const SCPStatement* st;
+
+    /// Ctor
+    public this (const SCPStatement* st) @nogc nothrow @trusted


Suggested change

public this (const SCPStatement* st) @nogc nothrow @trusted

public this (const SCPStatement* st) @nogc nothrow @safe pure

No ?

I did not really know ctors can be pure. I'm also surprised a function can be safe when assigning pointers. Oh well.

Geod24 · 2020-04-03T09:16:08Z

source/agora/consensus/protocol/Nominator.d

+        hashPart(this.st.slotIndex, dg);
+        hashPart(this.st.pledges.type_, dg);
+
+        switch (this.st.pledges.type_)


Suggested change

switch (this.st.pledges.type_)

final switch (this.st.pledges.type_)

I was gonna say that final switch sucks because of https://issues.dlang.org/show_bug.cgi?id=11051

But then I saw that the issue had a PR.. dlang/dmd#6095

lol

Geod24 · 2020-04-03T09:17:16Z

source/agora/consensus/protocol/Nominator.d

+    private const SCPEnvelope* env;
+
+    /// Ctor
+    public this (const SCPEnvelope* env) @nogc nothrow @trusted


Suggested change

public this (const SCPEnvelope* env) @nogc nothrow @trusted

public this (const SCPEnvelope* env) @nogc nothrow @safe pure

Necessary prerequisite for signing SCP messages.

AndrejMitrovic · 2020-04-03T09:25:19Z

Fixed again.

Geod24 · 2020-04-03T11:35:41Z

Drey 0 - Travis 1

AndrejMitrovic added the type-enhancement An improvement of existing functionalities label Feb 17, 2020

AndrejMitrovic added this to the 2. Validator milestone Feb 17, 2020

AndrejMitrovic mentioned this pull request Feb 17, 2020

Sign SCP envelopes and add a Byzantine node test #548

Closed

AndrejMitrovic force-pushed the hash-statements branch from 01bfd25 to 0d0fbb4 Compare February 18, 2020 08:30

Geod24 removed this from the 2. Validator milestone Mar 10, 2020

AndrejMitrovic force-pushed the hash-statements branch from 0d0fbb4 to 5208e8b Compare April 3, 2020 01:47

AndrejMitrovic mentioned this pull request Apr 3, 2020

Consensus failure test-case #716

Closed

AndrejMitrovic force-pushed the hash-statements branch from 5208e8b to bb21f30 Compare April 3, 2020 03:36

Geod24 reviewed Apr 3, 2020

View reviewed changes

AndrejMitrovic force-pushed the hash-statements branch from bb21f30 to 3e936a8 Compare April 3, 2020 07:03

AndrejMitrovic changed the title ~~Add support for hashing SCPStatements~~ Add support for hashing SCPStatement & SCPEnvelope Apr 3, 2020

AndrejMitrovic force-pushed the hash-statements branch 3 times, most recently from 5b287cc to f4588b4 Compare April 3, 2020 07:57

AndrejMitrovic force-pushed the hash-statements branch from f4588b4 to 6ebdc4e Compare April 3, 2020 09:09

AndrejMitrovic mentioned this pull request Apr 3, 2020

Examine and fix all usages of toVec() #723

Closed

Geod24 approved these changes Apr 3, 2020

View reviewed changes

AndrejMitrovic added 2 commits April 3, 2020 18:24

Add support for hashing SCPStatement

0f92fa9

Necessary prerequisite for signing SCP messages.

Add support for hashing SCPEnvelope

1d20c65

AndrejMitrovic force-pushed the hash-statements branch from 6ebdc4e to 1d20c65 Compare April 3, 2020 09:25

Geod24 merged commit 94f8ee8 into bosagora:v0.x.x Apr 3, 2020

AndrejMitrovic mentioned this pull request Apr 6, 2020

Design and implement quorum balancing #240

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for hashing SCPStatement & SCPEnvelope #546

Add support for hashing SCPStatement & SCPEnvelope #546

AndrejMitrovic commented Feb 17, 2020 •

edited

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading

AndrejMitrovic commented Apr 3, 2020

Geod24 Apr 3, 2020

Geod24 Apr 3, 2020

Geod24 Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

Geod24 left a comment

Geod24 Apr 3, 2020

AndrejMitrovic Apr 3, 2020

Geod24 Apr 3, 2020

AndrejMitrovic Apr 3, 2020

Geod24 Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

Geod24 commented Apr 3, 2020


		***************************************************************************/

		public void computeHash (const ref SCPStatement._pledges_t._prepare_t prep,

	public this (const SCPStatement* st) @nogc nothrow @trusted
	public this (const SCPStatement* st) @nogc nothrow @safe pure

	switch (this.st.pledges.type_)
	final switch (this.st.pledges.type_)

	public this (const SCPEnvelope* env) @nogc nothrow @trusted
	public this (const SCPEnvelope* env) @nogc nothrow @safe pure

Add support for hashing SCPStatement & SCPEnvelope #546

Add support for hashing SCPStatement & SCPEnvelope #546

Conversation

AndrejMitrovic commented Feb 17, 2020 • edited Loading

codecov bot commented Feb 18, 2020 • edited Loading

Codecov Report

AndrejMitrovic commented Apr 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

Geod24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrejMitrovic commented Apr 3, 2020

Geod24 commented Apr 3, 2020

AndrejMitrovic commented Feb 17, 2020 •

edited

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading