-
Notifications
You must be signed in to change notification settings - Fork 23
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 the quorum intersection checker code from stellar-core #664
Conversation
Codecov Report
@@ Coverage Diff @@
## v0.x.x #664 +/- ##
==========================================
- Coverage 91.34% 91.32% -0.02%
==========================================
Files 65 65
Lines 5162 5167 +5
==========================================
+ Hits 4715 4719 +4
- Misses 447 448 +1
Continue to review full report at Codecov.
|
a314020
to
39eec98
Compare
The part that assigns unassigned nodes needs to be removed. Not all nodes need to be in other nodes' quorums. |
687622b
to
f740fb1
Compare
f740fb1
to
0d1bf7d
Compare
I rebased it on v0.x.x, let's see if it passes on Windows |
Looks nice (and green). Will give it a review today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed self-review.
source/scpd/quorum/QuorumTracker.d
Outdated
ref const(QuorumMap) getQuorum() const; | ||
} | ||
|
||
private void quorum_tracker_rebuild (void* quorum_tracker, void* cb, void* dg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ref
and the proper type instead of void*
source/scpd/quorum/QuorumTracker.d
Outdated
quorum_tracker_rebuild(cast(void*)this, &call, cast(void*)&lookup); | ||
} | ||
|
||
// returns the current known quorum |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
source/scpd/quorum/QuorumTracker.d
Outdated
bool expand(const ref NodeID id, SCPQuorumSetPtr qSet); | ||
|
||
// replaces 'rebuild' taking an std::function so we can use it with D delegates | ||
extern (D) public final void rebuild (SCPQuorumSetPtr delegate(const ref NodeID) lookup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern (D) public final void rebuild (SCPQuorumSetPtr delegate(const ref NodeID) lookup) | |
extern (D) public final void rebuild (SCPQuorumSetPtr delegate(const ref NodeID) lookup) @trusted |
And other attributes.
source/scpd/quorum/QuorumTracker.d
Outdated
|
||
extern (C++, `stellar`): | ||
|
||
// helper class to help track the overall quorum over time |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
// workaround to be able to pass D delegates to C++ | ||
void quorum_tracker_rebuild (void* qt, void* cb, void* dg) | ||
{ | ||
typedef SCPQuorumSetPtr (*Callback)(void*, NodeID const&); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
extern(D) static shared_ptr!QuorumIntersectionChecker create ( | ||
QuorumTracker.QuorumMap map) nothrow pure @trusted @nogc | ||
{ | ||
return create(*cast(void**)&map); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Bindings for quorum/QuorumIntersectionChecker.h | ||
|
||
Copyright: | ||
Copyright (c) 2019 BOS Platform Foundation Korea |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
extern(C++, class) public abstract class QuorumIntersectionChecker | ||
{ | ||
public: | ||
extern(D) static shared_ptr!QuorumIntersectionChecker create ( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
source/scpd/quorum/QuorumTracker.d
Outdated
return (*dg)(node_id); | ||
} | ||
|
||
quorum_tracker_rebuild(cast(void*)this, &call, cast(void*)&lookup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace pointers with references.
source/scpd/quorum/QuorumTracker.d
Outdated
// `id` was known and didn't have a quorumset | ||
// returns false on failure | ||
// if expand fails, the caller should instead use `rebuild` | ||
bool expand(const ref NodeID id, SCPQuorumSetPtr qSet); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not addressed
bool expand(const ref NodeID id, SCPQuorumSetPtr qSet); | |
bool expand (const ref NodeID id, SCPQuorumSetPtr qSet); |
source/scpp/README.md
Outdated
@@ -15,6 +15,8 @@ Some files have been modified after porting: | |||
|
|||
- Some `private` declarations were changed to `public`. This allows us to test size & layout checks for our D glue layer, for example: https://github.com/bpfkorea/agora/blob/76e4ca7d77b123ef867d3ce99c1c1b26afab761d/source/scpp/src/crypto/ByteSlice.h#L21 used in https://github.com/bpfkorea/agora/blob/76e4ca7d77b123ef867d3ce99c1c1b26afab761d/source/scpp/extra/DSizeChecks.cpp and https://github.com/bpfkorea/agora/blob/76e4ca7d77b123ef867d3ce99c1c1b26afab761d/source/scpp/extra/DLayoutChecks.cpp. | |||
|
|||
- The `src/quorum` folder adapts the Quorum intersection tool which is used in Stellar's Herder implementation (not SCP). The `update.d` script does not track these files yet, so they have to be kept manually in sync when updating the bindings. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I suggest adding a in-between commit which just checks in all the intersection checker code as-is, but without bindings. And change the commit author to be the Stellar devs. Otherwise it appears like the code was forged or stolen, and we don't want to leave that kind of impression on the community. |
const shared_ptr!SCPQuorumSet nullquorum; | ||
|
||
// we replace the use of std::pair with a fixed-length array | ||
size_t first (size_t[2] pair) { return pair[0]; } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
(((n * pct) - size_t(1)) / size_t(100))); | ||
} | ||
|
||
NodeID toStellarKey (PublicKey address) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
auto qic = QuorumIntersectionChecker.create(qm); | ||
assert(qic.networkEnjoysQuorumIntersection()); | ||
|
||
//CircularAppender().printConsole(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
} | ||
|
||
//TEST_CASE("quorum intersection 6-node with subquorums", | ||
// "[herder][quorumintersection]") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0d1bf7d
to
affed9d
Compare
949aa53
to
cdb98a1
Compare
Completed fixes after self-review. |
With regards to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah... Sorry it took me so long to review this. So I left a few comments. The most important ones are w.r.t. class
vs struct
, and the need for size checks which uncovered #753 .
A few more things:
- I didn't port the SCP tests when I imported the SCP code. The reasoning was that little change would be required to SCP (if any), and none of that will be of substance. Any substantial change should be submitted upstream, where its sanity could be checked. So basically piggybacking on upstream having proper tests. I was a bit surprised when I saw the porting of the tests, and would have not originally asked for it, unless you had major plans to change the code being imported (which I guess you do). But the reasoning I just mentioned was also intended to save us time, and if the work is done, I see no reason not to pull it.
- The last two commit should be merged. I appreciate the effort to make the diff readable (and it was much more pleasant that it could have been), but since
dub
compiles everything, there will be a commit that doesn't build. Also, I believegit
(/github) has greatly improved their rename tracking, so the diff might not be so bad (did you test it?). But atomic commit trumps readability in that case. - As mentioned above,
dub
compiles everything. You might want to add the tests to theexcludedSourceFiles
so is only compiled on unittests.
Given that we use the fake wrapper you mentioned in #753 this is good to go after the fixes. Just add the commented out size check and a comment explaining why it is not currently correct.
extern (C++, `stellar`): | ||
|
||
/// Ditto | ||
extern(C++, class) public abstract class QuorumIntersectionChecker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extern(C++, class) public abstract class QuorumIntersectionChecker | |
public abstract class QuorumIntersectionChecker |
import scpd.types.Stellar_types; | ||
import scpd.quorum.QuorumTracker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import scpd.types.Stellar_types; | |
import scpd.quorum.QuorumTracker; | |
import scpd.quorum.QuorumTracker; | |
import scpd.types.Stellar_types; |
source/scpd/quorum/QuorumTracker.d
Outdated
// `id` was known and didn't have a quorumset | ||
// returns false on failure | ||
// if expand fails, the caller should instead use `rebuild` | ||
bool expand(const ref NodeID id, SCPQuorumSetPtr qSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not addressed
bool expand(const ref NodeID id, SCPQuorumSetPtr qSet); | |
bool expand (const ref NodeID id, SCPQuorumSetPtr qSet); |
source/scpd/quorum/QuorumTracker.d
Outdated
|
||
Bindings for quorum/QuorumTracker.h | ||
|
||
Note: D code does not need to use anything in this class except the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: D code does not need to use anything in this class except the | |
D code does not need to use anything in this class except the |
Having Note:
will create a section which will look bad in the doc IIRC. If you really intended this, perhaps
Note:
D code does not [...]
is more appropriate
source/scpd/quorum/QuorumTracker.d
Outdated
this (SCP* scp); | ||
|
||
/// returns true if id is in transitive quorum for sure | ||
bool isNodeDefinitelyInQuorum(const ref NodeID id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, and neither are the following ones. In D those functions are virtual, while in C++ they are not. You can make either the affected functions final
, or, if the class doesn't have a single virtual
function, make the class
itself final.
From my experience, things declared as class
with no virtual
method => struct
. And even more so if the class
is stored as a member of another type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In D those functions are virtual
Oh damn. How come it worked?
From my experience, things declared as class with no virtual method => struct.
What about classes which inherit other classes? This one specifically is defined as class QuorumTracker : public NonMovableOrCopyable
, so it looks like it was never supposed to be used as a POD type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class QuorumTracker : public NonMovableOrCopyable
Sounds like the idiomatic way to do this in D would be:
mixin template NonMovableOrCopyable (T /* = typeof(this) */) // Not sure if the default arg works
{
@disable this(this);
@disable ref T opAssign () (auto ref T other);
}
public struct QuorumTracker
{
mixin NonMovableOrCopyable!(QuorumTracker);
}
That might be a bit of an overreaching statement, but my experience is that good, principled C++ code ends up being idiomatic D code, and trying to use the same construct in D as one does in C++ might lead one astray.
Oh damn. How come it worked?
Depends on the usage. The code called from C++ doesn't have an issue, obviously. The code called from D may or may not crash, depending on whether or not the compiler devirtualize the call. And since we dropped DMD, there is a good chance it does :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, the above snippet doesn't fully capture the C++ intent though, since in D, you cannot declare a struct
are non movable. Should probably include a copy constructor too but it currently triggers a cascade of bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since we dropped DMD, there is a good chance it does :)
LOL.
Yeah also I dislike virtual by default in D. Where is Manu when you need him.
source/scpd/quorum/QuorumTracker.d
Outdated
bool expand(const ref NodeID id, SCPQuorumSetPtr qSet); | ||
|
||
/// Lookup function to find a mapping from Node ID => Quorum set | ||
private alias CallbackType = SCPQuorumSetPtr delegate(const ref NodeID) @trusted @nogc nothrow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used
SCP* mSCP; | ||
QuorumMap mQuorum; | ||
|
||
public: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the usage of :
-attributes, but can we merge the two public:
together ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I dislike :
attributes personally. I'll fix up the mistake though.
import scpd.quorum.QuorumIntersectionChecker; | ||
import scpd.quorum.QuorumTracker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong order
I wanted to verify the binding was correct. It wasn't a big effort tho, and it did catch bugs in my binding. That being said I guess we don't really need it anymore. We will have separate tests for the quorums we generate, so maybe we can remove this code? It might even be complicated to have something which is Stellar-licensed in our codebase, but I'm not lawyer.
Yeah this was purely to make it easy to review. I'll squash them.
Oh right. Will fix. |
cdb98a1
to
1ca17e1
Compare
6af429b
to
5c49f1c
Compare
|
Also the block storage checksum thing failed on Windows again. Like I said in #731, why not just version it out on Windows and fix it later? |
Can I just get rid of the destructor? I don't know why it's defined on the C++ side: virtual ~QuorumIntersectionChecker(){}; |
It's defined so that there is an entry in the vtable for the destructor. Otherwise you end up with things like this. |
5c49f1c
to
66c7ff6
Compare
I don't understand where the submodule update is coming from.. |
See also: stellar/stellar-core#2127 Also made changes to make the Quorum checker buildable In stellar-core v11.2.0 the quorum intersection checker depends on Stellar's Config struct. This dependency was removed here so we can use the code. Additionally shared_ptr was removed from one of the parameters as it was causing linker issues, plus we do not have a complete binding to shared_ptr with the correct semantics yet. Co-authored-by: MonsieurNicolas <nicolas@stellar.org>
This allowed us to catch a few bugs with our bindings. Co-authored-by: AndrejMitrovic <andrej.mitrovich@gmail.com>
66c7ff6
to
5bf37db
Compare
Ok I managed to get rid of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
It also includes tests.
Requires:
Add rudimentary bindings to std::unordered_map #662Add the BitSet implementation from stellar-core #663