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

Backports from quorumnet branch for 5.x #872

Merged
merged 16 commits into from Oct 8, 2019

Conversation

@jagerman
Copy link

jagerman commented Oct 7, 2019

This backports various additions and cleanups from the quorumnet branch (PR #844) that we can use in 5.x. (The actual quorumnet communication layer itself is not included and will come in Loki 6.x/HF14).

Aside from the miscellaneous code cleanups/fixes/improvements, I've been mulling over whether to include the ed25519 keys for a while, and think we should: lokinet and storage server can potentially make use of them before HF14, so this adds ed25519 key generation and uptime proof broadcast for HF13.

This also (in addition to the ed25519 pubkey) adds a uint16_t placeholder for the quorumnet port in uptime proofs; it current is just set to 0 and ignored, but having it there means Loki 6.x nodes will be able to broadcast the port before HF14 so that at the HF14 block they will already have everything needed (ed25519 + qnet port) to immediately begin quorumnet communication. (Previously I was going to delay the start of quorum comms after the fork by half a day, but this seems nicer).

@Doy-lee - most of the changes here are already reviewed via #844, but there were some small cleanups that are worth including in the Code cleanup commit and Simplify memcpy with offset commit (everything else is unchanged from #844); the "Add SN ed25519/..." commit isn't a direct cherry-pick (as it needed some quorumnet code stripped out of it to apply here) but isn't substantially different from the same commit in #844.

This is going to break the validity of uptime proofs on the testnet, so we should coordinate upgrading testnet nodes with merging this PR.

@jagerman jagerman force-pushed the jagerman:qnet-backport branch from 60db2db to 3c45860 Oct 7, 2019
jagerman added 14 commits Sep 12, 2019
POD_CLASS is a retarded macro (it is always just `struct`).

The pack pragmas here do nothing because every type defined inside them
only has char array members which are already guaranteed by C++
alignment rules to have byte alignment.
This "class" is a hot mess of uselessness:

- It's defined as a class that has declared constructor/destructor/copy
  assignment, but can't be instantiated (because the declared
  constructors/destructors/etc. aren't defined anywhere).
- There's a bunch of completely useless `friend` declarations in the
  crypto wrappers (public_key, etc.) that do absolutely nothing since
  those wrappers don't *have* anything private in them.
- Every (private) static function of crypto_ops is completely duplicated
  outside crypto_ops taking exactly the same arguments.
- In order to call all those private static functions the containing
  namespace has inlined free functions calling the private static
  functions inside crypto_ops with identical arguments.
- Because the above isn't allowed, all of the definitions are repeated
  a *third* time with friend function declarations inside crypto_ops.

So basically everything inside crypto_ops is exactly callable outside
crypto_ops with the same name and same arguments, but has this
completely pointless layer of indirection that adds nothing for no
reason.

This commits rips out all this useless crap and just makes the various
previous-crypto_ops static functions plain functions in the `crypto`
namespace (which is the only place they can be used and called anyway).
Avoids an extra copy plus also allows move assignment where available.
- removed unused forward declaration
- fix class/struct forward declaration mismatch warning
This enables various nice things (some of which are used in the
quorumnet code), and C++14 is old enough now that it's supported more or
less everywhere.

Additionally other parts of loki (e.g. lokinet, loki-storage-server)
have required C++14 all along, so this doesn't really change what we
support.
We don't need to convert it to string for anything *except* sending it
as log output, so simplify it to match things like txversion.
This is very useful for testing.  Also adds a suitable scary warning at
startup when used.
Five years into the future this pointless method is still not used.
Time to go.
The system library C++ interface is an ancient version of this; this
vendors an updated copy that we need, and updates a couple existing
places that are using deprecated calls.
Lets you write `memcpy_le(dest, foo, bar, baz)` and memcpy the contents
of foo, bar, and baz sequentially.  Additionally, if any of those three
are integers, they are converted to little-endian order.
This puts the SN pubkey/privkey into a single struct (which have
ed25519/x25519 keys added to it in the future), which simplifies various
places to just pass the struct rather than storing and passing the
pubkey and privkey separately.
This generates a ed25519 keypair (and from it derives a x25519 keypair)
and broadcasts the ed25519 pubkey in HF13 uptime proofs.

This auxiliary key will be used both inside lokid (starting in HF14) in
places like the upcoming quorumnet code where we need a standard
pub/priv keypair that is usable in external tools (e.g. sodium) without
having to reimplement the incompatible (though still 25519-based) Monero
pubkey format.

This pulls it back into HF13 from the quorumnet code because the
generation code is ready now, and because there may be opportunities to
use this outside of lokid (e.g. in the storage server and in lokinet)
before HF14.  Broadcasting it earlier also allows us to be ready to go
as soon as HF14 hits rather than having to wait for every node to have
sent a post-HF14 block uptime proof.

For a similar reason this adds a placeholder for the quorumnet port in
the uptime proof: currently the value is just set to 0 and ignored, but
allowing it to be passed will allow upgraded loki 6.x nodes to start
sending it to each other without having to wait for the fork height so
that they can start using it immediately when HF14 begins.
- Replace a bunch of repetitive proof rejection rules with a macro +
  HF requirement version loop.
- Remove mostly unused MAX_KEY_IMAGES_PER_CONTRIBUTOR variable.  It has
  always been set to 1 (and so doesn't actually do anything).  If we
  wanted to increase it at this point we'd have to add multiple
  HF-guarded versions of it and HF-version guard the code anyway, so
  seems simpler to just drop it.
- Simplify the max contributor/max contribution logic and merge it in
  with the existing contributor code (this is made easier by the above
  dropped variable).
@jagerman jagerman force-pushed the jagerman:qnet-backport branch from 3c45860 to c85b018 Oct 8, 2019
bool m_service_node;
crypto::secret_key m_service_node_key;
crypto::public_key m_service_node_pubkey;
std::shared_ptr<service_node_keys> m_service_node_keys;

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Oct 8, 2019

Collaborator

Is there a reason this should be a shared ptr? It's extra complexity.

This comment has been minimized.

Copy link
@jagerman

jagerman Oct 8, 2019

Author

Yes: the null pointer captures "not a service node".

This comment has been minimized.

Copy link
@jagerman

jagerman Oct 8, 2019

Author

... but more importantly it's also shared with service_node_list.

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Oct 8, 2019

Collaborator

Also suggests the usual things you associate with smart pointers like non-trivial lifetimes, ownership complexity. But if it exists for the entirety of the program then there are no lifetimes or ownership you have to consider. I'd encode the bool in the struct and operator bool if you want the syntactic niceness, or boost optional or raw ptr it.

Its a heavy handed approach for taking advantage of nullability.

This comment has been minimized.

Copy link
@jagerman

jagerman Oct 8, 2019

Author

Right, I mispoke, the sharing with service_node_list was the more important reason.

This comment has been minimized.

Copy link
@jagerman

jagerman Oct 8, 2019

Author

As for heaviness: shared_ptr deferences are extremely light; the only overhead with it compared to a raw pointer is when you creating a copy (because of needing an atomic increment) or destroying once, but neither or those are done here (except for the initial setup and the final destruction).

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Oct 8, 2019

Collaborator

I understand shared_ptrs alleviates the cost by using atomics. The cost is just a lot cheaper using the closer to zero cost alternatives. Considering cache friendliness, code generation (excluding boost::optional I'm guessing), lifetimes and ownership.

I can't say I particularly agree though- it's true we only really take a hit on startup and shutdown and that we're not a real time program and accesses to the keys are infrequent enough.

This comment has been minimized.

Copy link
@jagerman

jagerman Oct 8, 2019

Author

I understand shared_ptrs alleviates the cost by using atomics

No: it uses atomics only on copying/creation/destruction. There is no atomic operation required for dereferencing; a smart pointer deference it is literally optimized to nothing more than a pointer dereference.

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Oct 8, 2019

Collaborator

Right and there are copies in the codebase as it is right now. In quorum_cop when you receive blocks when this could just be free. It is for sharing, but you're sharing a read only object.

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Oct 8, 2019

Collaborator

Anyway, there's a push to move forward with the 5.0.0 release. So gonna merge this in

src/cryptonote_core/service_node_list.cpp Outdated Show resolved Hide resolved
jagerman added 2 commits Oct 8, 2019
This required a bit of reworking - without the duplication we only have
a const iterator so can't reuse it later for a non-const iterator, so
store the found contributor by index instead.
Use an alias to simplify a little.
@Doy-lee Doy-lee merged commit 4db1f0d into loki-project:dev Oct 8, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@jagerman jagerman referenced this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.