Add pretty-printing of SCPEnvelope #637

AndrejMitrovic · 2020-03-06T10:11:29Z

I'll wait to see the report from codecov to see if every line has been covered.

Still some things left to do:

In some places the hash is still displayed as hexadecimal in full, we could use the HashFmt for this.
Deserialization of ConsensusData in this case is handled gracefully. But in the future when we start using hashes, we can just provide another lookup function and print out <tx not found> when we're missing a tx that's being nominated.

Fixes #635

codecov · 2020-03-09T01:30:39Z

Codecov Report

Merging #637 into v0.x.x will increase coverage by 0.06%.
The diff coverage is 91.47%.

@@            Coverage Diff             @@
##           v0.x.x     #637      +/-   ##
==========================================
+ Coverage   91.31%   91.37%   +0.06%     
==========================================
  Files          62       63       +1     
  Lines        4927     5101     +174     
==========================================
+ Hits         4499     4661     +162     
- Misses        428      440      +12

Flag	Coverage Δ
#integration	`50.81% <0.00%> (-3.11%)`	⬇️
#unittests	`90.20% <91.47%> (+0.10%)`	⬆️

Impacted Files	Coverage Δ
source/agora/common/Config.d	`90.84% <ø> (ø)`
source/agora/consensus/protocol/Nominator.d	`91.39% <ø> (ø)`
source/agora/utils/PrettyPrinter.d	`90.17% <90.62%> (-0.07%)`	⬇️
source/agora/utils/SCPPrettyPrinter.d	`91.66% <91.66%> (ø)`
source/agora/test/Base.d	`89.59% <0.00%> (+0.45%)`	⬆️
source/agora/common/Serializer.d	`96.34% <0.00%> (+0.45%)`	⬆️
source/agora/network/NetworkManager.d	`80.12% <0.00%> (+0.62%)`	⬆️

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 0c45867...d6511db. Read the comment docs.

AndrejMitrovic · 2020-03-09T02:18:00Z

Oh great, I love linker errors.

Geod24 · 2020-03-09T02:28:19Z

Oh great, I love linker errors.

Move all your function-scope imports to global scope.

AndrejMitrovic · 2020-03-09T03:44:16Z

Move all your function-scope imports to global scope.

Thanks that works. But I thought this was just an issue with RDMD, not dub..

AndrejMitrovic · 2020-03-09T03:45:37Z

Ah no it did not work. :>

Ok I will look into this..

Geod24 · 2020-03-09T04:50:47Z

Thanks that works. But I thought this was just an issue with RDMD, not dub..

It's actually an issue with DMD, and we use some of the dependency tracking magic in some places (also we use rdmd in tests/runner.d)

AndrejMitrovic · 2020-03-10T02:24:30Z

The linking issue is for the client configuration. But the client shouldn't be importing into PrettyPrinter, so I don't quite get why this is causing linker issues.

AndrejMitrovic · 2020-03-10T03:41:48Z

Ok I figured it out. Had to add "source/agora/utils/PrettyPrinter.d", to the excluded source files.

Geod24 · 2020-03-12T06:36:30Z

Looks like something in the integration test suite depends from the pretty printer tho

AndrejMitrovic · 2020-03-12T08:18:05Z

I've split the SCP pretty-printing into another module, and added several commits introducing ConsensusData and Enrollment pretty-printing support.

Geod24

Very nice, a few comments

dub.json

@@ -54,6 +54,8 @@
                "source/agora/common/Config.d",
                "source/agora/network/*",
                "source/agora/node/*",
+                "source/agora/utils/PrettyPrinter.d",
+                "source/agora/utils/SCPPrettyPrinter.d",


source/agora/utils/SCPPrettyPrinter.d

+import scpd.Cpp;
+import scpd.types.Stellar_SCP;
+import scpd.types.Stellar_types : StellarHash = Hash;
+alias StellarValue = Value;


AndrejMitrovic · 2020-03-12T08:52:56Z

source/agora/utils/SCPPrettyPrinter.d

+
+            if (qset !is null)
+            {
+                auto qconf = toQuorumConfig(*qset.ptr);


One was using alias this (shared_ptr) and the other explicit dereference. I'll simplify it.

Geod24 · 2020-03-12T08:46:08Z

source/agora/utils/SCPPrettyPrinter.d

+GCOQ...LRIJ(62,500,000), GCOQ...LRIJ(62,500,000)], enrolls: [{ utxo: 0x0000...e26f, seed: 0x4a5e...a33b, cycles: 1008, sig: 0x0000...be78 }, { utxo: 0x0000...e26f, seed: 0x4a5e...a33b, cycles: 1008, sig: 0x0000...be78 }] } }, prep: <null>, prepPrime: <null>, nc: 100, nH: 200 } }, sig: 0x0af7...b5ab }`;
+
+    // with quorum mapping
+    static immutable PrepareRes2 = `{ statement: { node: GBUV...KOEK, slotIndex: 0, pledge: SCP_PREPARE { qset: { hash: 0xfee13ea2fd956028d31b93cbf0a47ea97c6c9868b7e911b797667113ed12bf0d, quorum: QuorumConfig(2, [GBFDLGQQDDE2CAYVELVPXUXR572ZT5EOTMGJQBPTIHSLPEOEZYQQCEWN, GBYK4I37MZKLL4A2QS7VJCTDIIJK7UXWQWKXKTQ5WZGT2FPCGIVIQCY5], []) }, ballot: { counter: 42, value: { tx_set: [Type : Payment, Inputs (1): 0x0000...0000[0]:0x0000...0000


Should we abbreviate SCP_PREPARE.qset.hash ? Also QuorumConfig entries ?

Good ideas.

AndrejMitrovic · 2020-04-01T09:59:55Z

Update:

Renamed SCP_EXTERNALIZE to Externalize, etc.
Added formatting for: StellarHash (32 byte hash)
Added pretty-printing for QuorumConfig

AndrejMitrovic · 2020-04-01T10:00:17Z

I'll force push again to reorder the commits.

AndrejMitrovic · 2020-04-02T01:10:34Z

Fixed linking issues. I had to re-add PrettyPrinter to exclude files again.

AndrejMitrovic · 2020-04-02T01:14:17Z

Hmm. I think I should consider changing all the *Fmt structs in SCPPrettyPrinter to store a pointer rather than a value as a field, because we want to avoid passing C++ types by value.

@Geod24 what do you think?

Geod24 · 2020-04-02T01:20:22Z

That was my original approach, however it didn't work. The details of why escape me ATM. Might be something to do with @safe. But yeah, we definitely want this.

AndrejMitrovic · 2020-04-02T01:29:23Z

Oops. I pinged someone. :x

AndrejMitrovic · 2020-04-02T04:07:37Z

source/agora/common/Config.d(26,8): Error: module node is in file 'dyaml/node.d' which cannot be read

hmm..

AndrejMitrovic · 2020-04-03T07:39:26Z

Update: removed copying.

There's still the weird compilation failure on circle-ci.

AndrejMitrovic · 2020-04-03T07:44:54Z

I know how to fix it, I'll move QuorumConfig out of the Config module.

AndrejMitrovic · 2020-04-03T07:46:57Z

Updated.

It's disallowing passing rvalues.

To allow passing rvalues.

We need the ability to pass rvalues.

It will be used by PrettyPrinter, and we don't want to import the Config module just to be able to pretty-print the quorum data structure.

It includes pretty-printing for all the possible pledge types (prepare / commit / externalize / nominate), and also handles deserialization failures gracefully. It is in a separate module to avoid linking issues with the integration tests, which import into the PrettyPrinter module and should not import into SCP. Fixes bosagora#635

Geod24

Amazing, thanks!

AndrejMitrovic added the type-enhancement An improvement of existing functionalities label Mar 6, 2020

AndrejMitrovic force-pushed the pretty-scp branch from 327d342 to b39f5b3 Compare March 9, 2020 01:30

AndrejMitrovic force-pushed the pretty-scp branch from b39f5b3 to b486446 Compare March 9, 2020 03:39

AndrejMitrovic force-pushed the pretty-scp branch from cfa7e83 to 8f79756 Compare March 10, 2020 02:03

AndrejMitrovic force-pushed the pretty-scp branch from 8f79756 to 365c9dd Compare March 10, 2020 03:41

AndrejMitrovic changed the title ~~[wip] Add pretty-printing of SCPEnvelope~~ Add pretty-printing of SCPEnvelope Mar 10, 2020

MichaelKim20 mentioned this pull request Mar 12, 2020

System test: Resolve agora module dependencies for SCP. #654

Closed

AndrejMitrovic force-pushed the pretty-scp branch from 365c9dd to a961e1e Compare March 12, 2020 08:17

AndrejMitrovic force-pushed the pretty-scp branch from a961e1e to 4e80230 Compare March 12, 2020 08:27

Geod24 reviewed Mar 12, 2020

View reviewed changes

AndrejMitrovic force-pushed the pretty-scp branch from 4e80230 to 9b3a7b4 Compare April 1, 2020 09:17

AndrejMitrovic force-pushed the pretty-scp branch from 9b3a7b4 to 3f78987 Compare April 1, 2020 09:59

AndrejMitrovic force-pushed the pretty-scp branch 3 times, most recently from 4c17602 to 4f433a3 Compare April 2, 2020 01:10

AndrejMitrovic force-pushed the pretty-scp branch from 4f433a3 to e15c293 Compare April 3, 2020 07:39

AndrejMitrovic force-pushed the pretty-scp branch from e15c293 to cd66e2a Compare April 3, 2020 07:46

AndrejMitrovic added 8 commits April 6, 2020 08:39

Remove constructor from PubKeyFmt

c84995f

It's disallowing passing rvalues.

Remove constructor from HashFmt

6448152

To allow passing rvalues.

Change prettify to accept 'const auto ref'

9c4e3d4

We need the ability to pass rvalues.

Add pretty-printing for Enrollment

419c7fa

Add pretty-printer for ConsensusData

79f96dd

Move QuorumConfig to agora.common.Types

48a703b

It will be used by PrettyPrinter, and we don't want to import the Config module just to be able to pretty-print the quorum data structure.

Add pretty-printing for QuorumConfig

b9c9685

Geod24 force-pushed the pretty-scp branch from cd66e2a to d6511db Compare April 5, 2020 23:40

Geod24 approved these changes Apr 5, 2020

View reviewed changes

Geod24 merged commit 8b2ea0e into bosagora:v0.x.x Apr 5, 2020

AndrejMitrovic mentioned this pull request Apr 6, 2020

Design and implement quorum balancing #240

Closed

6 tasks

AndrejMitrovic deleted the pretty-scp branch April 6, 2020 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pretty-printing of SCPEnvelope #637

Add pretty-printing of SCPEnvelope #637

AndrejMitrovic commented Mar 6, 2020

codecov bot commented Mar 9, 2020 •

edited

Loading

AndrejMitrovic commented Mar 9, 2020

Geod24 commented Mar 9, 2020

AndrejMitrovic commented Mar 9, 2020

AndrejMitrovic commented Mar 9, 2020

Geod24 commented Mar 9, 2020

AndrejMitrovic commented Mar 10, 2020

AndrejMitrovic commented Mar 10, 2020

Geod24 commented Mar 12, 2020

AndrejMitrovic commented Mar 12, 2020

Geod24 left a comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

AndrejMitrovic Mar 12, 2020

Geod24 Mar 12, 2020

AndrejMitrovic Mar 12, 2020

AndrejMitrovic commented Apr 1, 2020

AndrejMitrovic commented Apr 1, 2020

AndrejMitrovic commented Apr 2, 2020

AndrejMitrovic commented Apr 2, 2020

Geod24 commented Apr 2, 2020

AndrejMitrovic commented Apr 2, 2020

AndrejMitrovic commented Apr 2, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

AndrejMitrovic commented Apr 3, 2020

Geod24 left a comment

Add pretty-printing of SCPEnvelope #637

Add pretty-printing of SCPEnvelope #637

Conversation

AndrejMitrovic commented Mar 6, 2020

codecov bot commented Mar 9, 2020 • edited Loading

Codecov Report

AndrejMitrovic commented Mar 9, 2020

Geod24 commented Mar 9, 2020

AndrejMitrovic commented Mar 9, 2020

AndrejMitrovic commented Mar 9, 2020

Geod24 commented Mar 9, 2020

AndrejMitrovic commented Mar 10, 2020

AndrejMitrovic commented Mar 10, 2020

Geod24 commented Mar 12, 2020

AndrejMitrovic commented Mar 12, 2020

Geod24 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

AndrejMitrovic Mar 12, 2020

Choose a reason for hiding this comment

Geod24 Mar 12, 2020

Choose a reason for hiding this comment

AndrejMitrovic Mar 12, 2020

Choose a reason for hiding this comment

AndrejMitrovic commented Apr 1, 2020

AndrejMitrovic commented Apr 1, 2020

AndrejMitrovic commented Apr 2, 2020

AndrejMitrovic commented Apr 2, 2020

Geod24 commented Apr 2, 2020

AndrejMitrovic commented Apr 2, 2020

AndrejMitrovic commented Apr 2, 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

codecov bot commented Mar 9, 2020 •

edited

Loading