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

Blink wallet burn #935

Merged
merged 103 commits into from Nov 29, 2019
Merged

Blink wallet burn #935

merged 103 commits into from Nov 29, 2019

Conversation

@jagerman
Copy link

jagerman commented Nov 26, 2019

(Just the last commit here is significant):

This revamps the loki_construct_tx_params a bit to be able to carry
fixed and %-based burn amounts (in addition to the staking tx flag) and
modifies it to set the hf version from within wallet2 rather than at
construction.

This is then used to implement proper fee burning in the wallet
(replacing the uncommitted hack I was using up until now to test it) by
changing wallet2 to understand the burn amount: initially it adds a
placeholder amount so that an appropriate amount of space in tx extra is
taken up, then once the transaction has been built and final fees
calculated it replaces the placeholder with the final burn amount before
tx finalization.

@Doy-lee

This comment has been minimized.

Copy link
Collaborator

Doy-lee commented Nov 27, 2019

After seeing the commit, it might be better to put the fee burning construction into construct_tx_with_tx_key reason being that this makes it really easy to support blink in core_tests which call through to said function and you only have to write the adding fee burn to the extra once (all TX construction eventually ends up in construct_tx_with_tx_key).

Something like

if (burn_permitted)
{
    remove_field_from_tx_extra(extra, typeid(tx_extra_burn));
    add_burned_amount_to_tx_extra(extra, burn);
}

You can determine the fee by using rctSigBase::txnFee amounts in - amounts out, before the RCT signing step at the bottom

@jagerman

This comment has been minimized.

Copy link
Author

jagerman commented Nov 27, 2019

After seeing the commit, it might be better to put the fee burning construction into construct_tx_with_tx_key reason being that this makes it really easy to support blink in core_tests which call through to said function and you only have to write the adding fee burn to the extra once (all TX construction eventually ends up in construct_tx_with_tx_key).

That feels a little weird given that the dummy value still has to be added in wallet2 (to make sure we don't construct txes that end up too large because of adding the extra field).

@Doy-lee

This comment has been minimized.

Copy link
Collaborator

Doy-lee commented Nov 27, 2019

Oh so staking should be doing this as well. Although that size check relies on just using extra.size(), which we could.. calculate independently.

jagerman added 27 commits Sep 17, 2019
Adds the zmq-based quorumnet communication layer.

Note that this is only essentially isolated library code in this commit
(which doesn't depend on anything else in loki).  The glue code that
ties them together follows in later commits.
This adds vote relaying via quorumnet.

- The main glue between existing code and quorumnet code is in
  cryptonote_protocol/quorumnet.{h,cpp}
- Uses function pointers assigned at initialization to call into the
  glue without making cn_core depend on p2p
- Adds ed25519 and quorumnet port to uptime proofs and serialization.
- Added a second uptime proof signature using the ed25519 key to that
  SNs are proving they actually have it (otherwise they could spoof
  someone else's pubkey).
- Adds quorumnet port, defaulting to zmq rpc port + 1.  quorumnet
  listens on the p2p IPv4 address (but broadcasts the `public_ip` to the
  network).
- Derives x25519 when received or deserialized.
- Converted service_info_info::version_t into a scoped enum (with the
  same underlying type).
- Added contribution_t::version_t scoped enum.  It was using
  service_info_info::version for a 0 value which seemed rather wrong.

Random small details:
- Renamed internal `arg_sn_bind_port` for consistency
- Moved default stagenet ports from 38153-38155 to 38056-38058, and add the default stagenet
  quorumnet port at 38059 (this keeps the range contiguous; otherwise the next stagenet default port
  would collide with the testnet p2p port).
For some reason they use GLOB for sources but this is strongly
discouraged.  This commit explicitly lists the files, just like every
other CMakeLists.txt in the project.
Something was apparently pulling this in already on my local system, but
the travis builds failed.
The map lookup is only really needed here for supporting multiple
SNNetwork instances, which is not going to be a common case.  This adds
an optimization for the common singleton case (which probably incurs a
small penalty for the multiple-SNNetwork case, but we aren't even
currently using that in loki).
The mismatch due to the offset between internal parts we have and the
parts we were actually sent from the remote looked wrong at first
glance; this commit makes it more obvious.
Quorumnet channels are bidirectional, which means we want to be able to
broadcast info to nodes that connect to us (if they have done so).  This
paves the way by providing an iterator through incoming connection
quorum members.
Also moves it into `quorumnet` namespace (instead of
`quorumnet::detail`).
But only non-temporary std::string lvalues, *not* temporaries since they
completely defeat the point.

Also beefs up the class documentation.
Unify the field we use to store the count as `_count` (using the leading
underscore to indicate a private value rather than an intended enum
value) and add/use a new `enum_count` template variable to extract the
_count enum value and cast it to the enum's underlying_type.
This adds an ability to do an "optional" send, that is, a send that only
goes out if we're already connected to the given remote.

This also allows send() to take any number of data arguments of any
serializable type instead of only bt_dict arguments; each one becomes an
appended serialized message part.  This means you can now do something
like:

    std::unordered_map<std::string, int> foo{{"hello",1}, {"world",42}};
    sn.send(pubkey, "helloworld", foo);

or:

    sn.send(pubkey, "theanswer", 42);

or any other serializable data structure instead of having to use the
added complexity of a bt_dict.

This also changes send_hint (and a hypothetical send_optional) to be
passed into send() as simple option structs:

    sn.send(pubkey, "helloworld", foo, send_option::optional{}, send_option::hint("tcp://localhost:1234"));

rather than needing send, send_hint, send_optional, and potential other
future send options.  (These options and data values can be given in any
order).

To go along with the arbitrary send data, this also converts the
`message` encapsulation to have a vector of `bt_value`s rather than a
single `bt_dict`.

This also means send() and handlers no longer eat empty data structures:
`sn.send(pk, cmd)` and `sn.send(pk, cmd, dict)` with dict empty now
deliver difference values (specifically an empty data vector, and a
1-element data vector containing a dict).
Separation of responsibility: service_nodes doesn't need to store an
rpc-specific value.
Matches != {0}, just like bool operator on public keys.
testing_quorum is a bit of a misnomer: it only does testing when used
for an obligations quorum, but it is also used for checkpointing and
soon Blink.
jagerman added 22 commits Nov 17, 2019
None of this code is used or called or invoked anywhere, yet complicates
things by throwing in unneeded multiple inheritance, a unique_ptr that
is never touched, and compiling functions (and an extra .cpp file) that
are never called which are full of FIXMEs and TODOs and probably don't
work.  None of this has been touched in years, either.

Purge it.
- Simplify the algorithm (while keeping the results the same)
- Don't use a return value for a function that doesn't have a meaningful
  return value
- Clarify and correct documentation of what it does
Everything that uses these structs already either zero initializes or
doesn't need to zero initialize, so the epee stuff is just annoying and
provides no benefit (plus some of the additions don't use the epee
misdirection layer already).
This allows fetching multiple tx heights in a single tx rather than
needing to call multiple times and incur the overhead of a transaction.

This devirtualizes the singular version and reimplements it as a simple
wrapper around the vector version.
This adds blink tx synchronization: when doing a periodic sync with
other nodes each node includes a map of {HEIGHT, HASH} pairs where
HEIGHT is a mined, post-immutable block height and HASH is an xor of all
the tx hashes mined into that block.

If upon receipt the node disagrees about what it thinks the HASH should
be, it can request a list of txes for one or more disagreeing heights,
for which it gets a list of tx hashes of all blink txes mined into that
block.

If it is then missing any of those TXes, this adds an ability to request
that the remote send TXes via the existing NOTIFY_NEW_TRANSACTIONS
mechanism, but with an added flag (so that these don't get relayed).

This originally was going to request the TXes via the existing
NOTIFY_REQUEST_GET_OBJECTS, which has a `txs` field, but unfortunately
any txs passed to it are completely ignored; it is *only* usable for
block synchronization.  As such I renamed it and removed the `txs` field
to make the responsibility/capability clearer.
- actually include the blink hashes in the core sync data
- fix cleanup to delete heights in (0, immutable] instead of [0,
immutable); we want to keep 0 because it signifies the mempool, and we
only need blocks after immutable, not immutable itself.
- fixed NOTIFY_REQUEST_GET_TXS to handle mempool txes properly (it was
only searching the chain and ignored missed_txs, but missed_txs are ones
we need to look up in the mempool)
- Add a method to tx_pool (needed for the above) to grab multiple txes
by hash (essentially a multi-tx version of `get_transaction()`), and
change get_transaction() to use it with a single value.
- Added various debugging statements
- Added a bunch of comments to each condition of the preliminary blink
data check condition.
- Don't abort blink addition on a single signature failure: if there are
enough valid signatures we should still accept it.
- Check for blink signature approval when receiving blink signatures;
it's not enough to know that all were added successfully, we also have
to ask the blink tx if it is approved (which does additional checks on
subquorum counts) once we add them all.
It's helpful to know which of these is triggered.
Blink status was only being checked on get_transactions rpc for
already-mined txes, but needs to be checked for mempool txes too.
Retrieving missing blinks are now done on a per-peer basis rather than
globally.
…ights

This makes get_blink_hashes_and_mined_heights filter out blinks in
immutable blocks; since we have to find them all here in order to do
that (and because this is regularly called) it's also a good place to
expire old blink entries.
Extracts the tx removal lambda into a new method.

DRYs the nearly identical prune loop bodies (the only difference between
them is the iteration order).

Fix undefined behaviour in mempool pruning: on removal of a standard tx
the iterator was updated to the result of `.erase()`, but that's wrong
because the loop is going in backwards order, and so the first prune
would leave the iterator pointing at `.end()` which would be deferenced
in the next iteration.  The (new, DRY) lambda now iterates in the
desired direction.

This also makes (and uses) blockchain and tx_pool implement the standard
library Lockable concept by adding a `try_lock()` so that they can be
locked simultaneously.  It also updates them to hold a
boost::recursive_mutex directly rather than going through the pointless
and horribly designed epee `critical_section` wrapper (seriously who
thinks that making a mutex class copyable where a copy holds a new,
entirely unrelated mutex is a good idea?)
To make it obvious (and keep it consistent) with all the other
blink-related methods having "blink" in their name.

Also fix grammar in a related debug message.
We don't want to prune these because that would let someone spam the tx
pool to quickly get rid of a blink tx.
It is not needed: crypto::hash is directly comparable.
This adds support for bumping conflicting transactions from the mempool
upon receipt of a blink tx if those conflicting txs are not themselves
blinks.

This commit chops the handle_incoming_txs effectively in half into a
parse_incoming_txs phase, followed by a handle_incoming_txs phase which
is needed so that we can first parse the txes then, assuming they parse
successfully, can flag them as blink txes if blink info was included
with them, so that the actual insertion becomes forcing.

There is a stub here for also allowing a rollback of the mutable part of
the chain; implementation of actually doing that rollback will follow in
another commit.
Rename handle_incoming_txs to handle_parsed_txs, and re-add a
handle_incoming_txs that does the parse+handle_parsed call with the
appropriate lock held for code that doesn't care about being able to
split it into two steps.

Clarifies the locking requirements with a comment in the code, and adds
an incoming tx lock over the blink receiving process in
cryptonote_protocol_handler.inl.

Also fixes a bug in the weight calculation happening because the
blobdata pointer was being invalidated when going through
the singular `handle_parsed_tx()`.
The keep-pruning size condition was inverted.
This revamps the `loki_construct_tx_params` a bit to be able to carry
fixed and %-based burn amounts (in addition to the staking tx flag) and
modifies it to set the hf version from within wallet2 rather than at
construction.

This is then used to implement proper fee burning in the wallet
(replacing the uncommitted hack I was using up until now to test it) by
changing wallet2 to understand the burn amount: initially it adds a
placeholder amount so that an appropriate amount of space in tx extra is
taken up, then once the transaction has been built and final fees
calculated it replaces the placeholder with the final burn amount before
tx finalization.
@jagerman jagerman force-pushed the jagerman:blink-wallet-burn branch from 83eb4fb to 9b624e6 Nov 27, 2019
jagerman added 5 commits Nov 27, 2019
This allows anything to add burn amounts by specifying burn_fixed in the
loki_tx_params.

burn_percent is more complicated (because it is a percentage of the base
tx fee, not a percentage of the actual tx fee), and gets handled and
converted to a fixed amount in the wallet code.
(ZMQ_LIB added here includes both itself plus sodium.)
@Doy-lee Doy-lee merged commit 8df27d9 into loki-project:dev Nov 29, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
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.