Blink #915
Merged
Blink #915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
jagerman
force-pushed
the
blink
branch
2 times, most recently
from
64c349b
to
2474fbf
Compare
Doy-lee
reviewed
Nov 7, 2019
Doy-lee
reviewed
Nov 7, 2019
Doy-lee
reviewed
Nov 8, 2019
Doy-lee
reviewed
Nov 19, 2019
Doy-lee
reviewed
Nov 19, 2019
src/wallet/wallet2.cpp
Outdated
| } | ||
| else if (m_transfers[kit->second].m_spent || m_transfers[kit->second].amount() >= tx_scan_info[o].amount) | ||
| auto &transfer = m_transfers[kit->second]; | ||
| if (transfer.m_unmined_blink && !pool && transfer.amount() == tx_scan_info[o].amount) |
There was a problem hiding this comment.
There's sort of a loop hole here using the burning bug where you can immediately replace an older unspent output with a blink and make the output unusable because it sets the global output index to 0 and doesn't set m_unmined_blink to the appropriate value.
- Construct a non-blink transaction to the recipient, preserve the tx private key you generate for the recipient
- Construct a blink transaction with an amount greater than the previous sent amount, reusing the tx private key to send to the same stealth address.
Because the new output's amount > prior amount the else case triggers in this if/else chain. In that case we are replacing our pre-existing transfer in the wallet with the new incoming output because the new output's amount is greater than the one currently in the wallet. The new output is from blink- we replace the information in the transfer entry and also update transfer.m_global_output_index = pool ? 0 : o_indices[o];
But here we are replacing a pre-existing normal transaction, i.e. the transfer is not marked as a blink. When we go try and spend it, the wallet will try and reference the 0th global output and fail because presumably you don't hold the keys for that output.
A suggested fix might look like adding a bunch of sanity checks. My diff also includes folding down the first two if cases that repeat the same steps.
- disallow blinks replacing blinks (blinks should be immutable)
- disallow mined txs replacing unmined-blinks with different amounts
- allow blinks to replace older mined transactions iff it transfers an amount greater than the old amount
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 7d82b2a1f..090198c49 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -1927,10 +1927,12 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
+ "got " + (kit == m_pub_keys.end() ? "<none>" : boost::lexical_cast<std::string>(kit->second))
+ ", m_transfers.size() is " + boost::lexical_cast<std::string>(m_transfers.size()));
+ bool process_transaction = !pool || blink;
+ bool unmined_blink = pool && blink;
if (kit == m_pub_keys.end())
{
uint64_t amount = tx.vout[o].amount ? tx.vout[o].amount : tx_scan_info[o].amount;
- if (!pool || blink)
+ if (process_transaction)
{
pk_to_unlock_times[tx_scan_info[o].in_ephemeral.pub] = tx_scan_info[o].unlock_time;
@@ -1938,8 +1940,8 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
transfer_details& td = m_transfers.back();
td.m_block_height = height; // NB: will be zero for a blink; we update when the blink tx gets mined
td.m_internal_output_index = o;
- td.m_global_output_index = pool ? 0 : o_indices[o]; // blink tx doesn't have this; will get updated when it gets into a block
- td.m_unmined_blink = pool && blink;
+ td.m_global_output_index = unmined_blink ? 0 : o_indices[o]; // blink tx doesn't have this; will get updated when it gets into a block
+ td.m_unmined_blink = unmined_blink;
td.m_tx = (const cryptonote::transaction_prefix&)tx;
td.m_txid = txid;
td.m_key_image = tx_scan_info[o].ki;
@@ -2005,39 +2007,42 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
notify = true;
continue;
}
+
+ // NOTE: Pre-existing transfer already exists for the output
auto &transfer = m_transfers[kit->second];
- if (transfer.m_unmined_blink && !pool && transfer.amount() == tx_scan_info[o].amount)
+ THROW_WALLET_EXCEPTION_IF(blink && transfer.m_unmined_blink,
+ error::wallet_internal_error,
+ "A blink replacing an unmined blink should not happen, when a transaction is mined, "
+ "blink metadata is dropped and the TX should just be a normal TX");
+ THROW_WALLET_EXCEPTION_IF(pool && transfer.m_unmined_blink,
+ error::wallet_internal_error,
+ "An output replacing a unmined blink output must not be from the pool.");
+
+ if (transfer.m_spent || transfer.amount() >= tx_scan_info[o].amount)
{
- MINFO("Public key " << epee::string_tools::pod_to_hex(kit->first)
- << " of blink tx " << transfer.m_txid << " (for " << print_money(tx_scan_info[o].amount) << ")"
- << " status updated: now mined in block " << height);
+ if (transfer.amount() > tx_scan_info[o].amount)
+ {
+ LOG_ERROR("Public key " << epee::string_tools::pod_to_hex(kit->first)
+ << " from received " << print_money(tx_scan_info[o].amount) << " output already exists with "
+ << (transfer.m_spent ? "spent" : "unspent") << " "
+ << print_money(transfer.amount()) << " in tx " << transfer.m_txid << ", received output ignored");
+ }
- blink_got_mined = true;
+ if (transfer.m_unmined_blink)
+ {
+ blink_got_mined = true;
+ THROW_WALLET_EXCEPTION_IF(transfer.amount() != tx_scan_info[o].amount, error::wallet_internal_error, "A blink should credit the amount exactly as we recorded it when it arrived in the mempool");
+ THROW_WALLET_EXCEPTION_IF(transfer.m_spent, error::wallet_internal_error, "Blink can not be spent before it is mined, this should never happen");
- // We previous had this as a blink, but now it's been mined so update the tx status with the height and output index
- transfer.m_block_height = height;
- transfer.m_global_output_index = o_indices[o];
- transfer.m_unmined_blink = false;
+ MINFO("Public key " << epee::string_tools::pod_to_hex(kit->first)
+ << " of blink tx " << transfer.m_txid << " (for " << print_money(transfer.amount()) << ")"
+ << " status updated: now mined in block " << height);
- auto iter = std::find_if(
- tx_money_got_in_outs.begin(),
- tx_money_got_in_outs.end(),
- [&tx_scan_info,&o](const tx_money_got_in_out& value)
- {
- return value.index == tx_scan_info[o].received->index &&
- value.amount == tx_scan_info[o].amount &&
- value.unlock_time == tx_scan_info[o].unlock_time;
- }
- );
- THROW_WALLET_EXCEPTION_IF(iter == tx_money_got_in_outs.end(), error::wallet_internal_error, "Could not find the output we just added, this should never happen");
- tx_money_got_in_outs.erase(iter);
- }
- else if (transfer.m_spent || m_transfers[kit->second].amount() >= tx_scan_info[o].amount)
- {
- LOG_ERROR("Public key " << epee::string_tools::pod_to_hex(kit->first)
- << " from received " << print_money(tx_scan_info[o].amount) << " output already exists with "
- << (m_transfers[kit->second].m_spent ? "spent" : "unspent") << " "
- << print_money(m_transfers[kit->second].amount()) << " in tx " << m_transfers[kit->second].m_txid << ", received output ignored");
+ // We previous had this as a blink, but now it's been mined so update the tx status with the height and output index
+ transfer.m_block_height = height;
+ transfer.m_global_output_index = o_indices[o];
+ transfer.m_unmined_blink = false;
+ }
auto iter = std::find_if(
tx_money_got_in_outs.begin(),
@@ -2110,13 +2115,14 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
if (iter->amount == 0)
tx_money_got_in_outs.erase(iter);
- uint64_t amount = tx.vout[o].amount ? tx.vout[o].amount : tx_scan_info[o].amount;
+ uint64_t amount = tx.vout[o].amount ? tx.vout[o].amount : tx_scan_info[o].amount;
uint64_t extra_amount = amount - transfer.amount();
- if (!pool || blink)
+ if (process_transaction)
{
transfer.m_block_height = height;
transfer.m_internal_output_index = o;
- transfer.m_global_output_index = pool ? 0 : o_indices[o]; // blink tx doesn't have this; will get updated when it gets into a block
+ transfer.m_global_output_index = unmined_blink ? 0 : o_indices[o]; // blink tx doesn't have this; will get updated when it gets into a block
+ transfer.m_unmined_blink = unmined_blink;
transfer.m_tx = (const cryptonote::transaction_prefix&)tx;
transfer.m_txid = txid;
transfer.m_amount = amount;
Doy-lee
reviewed
Nov 20, 2019
Comment on lines
+974
to
+975
| // FIXME - am I 100% sure I want to do the above? Verifying the TX would cut off being able to | ||
| // induce a node to broadcast a junk TX to other quorum members. |
There was a problem hiding this comment.
After some thought, I feel like we want this. This seems too abusable. It sort of work outs if you just do handle_incoming_tx and set do_not_relay to true.
Then your added set_relayable() to make it relayable once the signatures have arrived.
Currently calling tx_memory_pool::add_tx directly doesn't check if the tx exists in the blockchain yet (but going through core::handle_incoming_tx does, though that appears to be the only check missing when going straight to tx_memory_pool::add_tx)
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.
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.
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.
This adds a tx extra field that specifies an amount of the tx fee that must be burned; the miner can claim only (txnFee - burnFee) when including the block. This will be used for the extra, burned part of blink fees and LNS fees and any other transaction that requires fee burning in the future.
Neither of these have a place in modern C++11; boost::value_initialized
is entirely superseded by `Type var{};` which does value initialization
(or default construction if a default constructor is defined). More
problematically, each `boost::value_initialized<T>` requires
instantiation of another wrapping templated type which is a pointless
price to pay the compiler in C++11 or newer.
Also removed is the AUTO_VAL_INIT macro (which is just a simple macro
around constructing a boost::value_initialized<T>).
BOOST_FOREACH is a similarly massive pile of code to implement
C++11-style for-each loops. (And bizarrely it *doesn't* appear to fall
back to C++ for-each loops even when under a C++11 compiler!)
This removes both entirely from the codebase.
This allows the caller to also take the response by rvalue reference so that they can move outsubvalues. The rvalue is totally fine here (once the callback is invoked it is never used again) and still binds perfectly well to const-lvalue accepting callbacks.
Removes one unnecessary layer of templated indirection in kv serialization, and removes use of boost::mpl::vector code generation. Also removes a double-specification of the same type in the epee array_entry specification.
We don't impose any alignment on hashable types, but this means the hashing function is doing invalid misaligned access when converting to a size_t. This aligns all of the primitive data types (crypto::hash, public keys, etc.) to the same alignment as size_t. That cascades into a few places in epee which only allow byte spanning types that have byte alignment when what it really requires is just that the type has no padding. In C++17 this is exactly the purpose of std::has_unique_object_representations, but that isn't available (or even implementable) in C++14 so add specializations for the type that need it to tell epee that we know those types are properly packed and that it can safely use them as bytes. Related to this, monero/epee also misuses `is_standard_layout` when the purpose is actually `is_trivially_copyable`, so fixed that too. (You need the latter but don't need the former for a type to be safely memcpy'able; the only purpose of `is_standard_layout` is when you need to be sure your structs are compatible with C structs which is irrelevant here).
These templates aren't instantiable with anything except for these exact argument types, so don't use a template.
This adds a thread-local, pre-seeded rng at `tools::rng` (to avoid the multiple places we are creating + seeding such an RNG currently). This also moves the portable uniform value and generic shuffle code there as well as neither function is specific to service nodes and this seems a logical place for them.
There is nothing gained at all by trying to pad the data to an even multiple of 1024 but it adds quite a lot of complexity (which would get worse with blink). Just adding some 0-1023 random bytes prevents traffic analysis just as well and considerably simplifies the code.
Pool printing is separately implemented in rpc_command_executor; this method in tx_pool is both obsolete (missing some fields) and not called anywhere.
The only difference between long and short is one string (the json info). Also remove std::endl because using it like this indicates that the author doesn't understand what std::endl is for. Also removed a completely ridiculous inline lambda.
The default copy and move constructors would be broken and dangerous; delete the copy ctor and provide a usable move ctor. Also explicitly delete copy and move assignment (they are implicitly deleted already but making this explicit signals intent). We can also save a bit of indirection by storing the BlockchainDB reference directly rather than the Blockchain (which isn't needed except to get the db). Also remove the `m_active` variable: the two bools here are doing nothing that a single m_batch bool can't do.
This code was too convoluted not to fix. - There are 5 linear searches done over 2 vectors (without any changes done to the vectors in between). - One of the linear searches calls out to epee to convert the same value to a hex string for every element. - Both a set and a map are used here with identical key contents. The old code looks like it only conditionally adds to the map if it finds a match in the loop, but the for loop is doing exactly the same test as the `find_if` that guards the entire `else if` containing it, so it will *always* match. - Removed some error code that can never actually run (because of the above set/map equivalence).
These structures will always end up packed anyway (everything in them is 8-byte aligned and a multiple of 8 bytes), but the pack pragma also changes their alignment which isn't good (and causes warnings). This just static_asserts that they are padding-free instead, which is the intention.
There are a bunch trivial forwarding wrappers in cryptonote_core that simply call the same method in the pool, and blink would require adding several more. Instead of all of these existing (and new) wrappers, just directly expose the tx_pool reference so that anything with a `core` reference can access and call to the mempool directly.
- Adds blink signature synchronization and storage through the regular p2p network - Adds wallet support (though this is still currently buggy and needs additional fixes - it sees the tx when it arrives in the mempool but isn't properly updating when the blink tx gets mined.)
- confirmed was erroneously being set to true for mempool transactions causing it to sort at the beginning instead of the end of the list. (Since it gets zero initialized just delete it) - show_transfers `min_height` was for some inexpicable reason being treated as a strict inequality so that `show_transfers 123 456` would not show you transactions from height 123. (The max_height was *not* strict, so you *do* get transactions from height 456).
Rename inner `brd` variable to avoid shadowing the outer scope `brd` variable.
This catches any exception thrown in the inner quorumnet blink code and sets it in the promise if it occurs, which propagates it out to core_rpc_server to catch and deal with.
- set the proper promise - reuse reference rather than repeating lookups - fix unneeded mempool lock
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the bulk of the work for blink. There are two pieces yet to come
which will follow in the next day or two which are: the p2p communication of blink
transactions (which needs to be fully synchronized, not just shared,
unlike regular mempool txes); and an implementation of fee burning.
Blink approval, multi-quorum signing, cli wallet and node support for
submission denial are all implemented here.
This overhauls and fixes various parts of the SNNetwork interface to fix
some issues (particularly around non-SN communication with SNs, which
wasn't working).
There are also a few sundry FIXME's and TODO's of other minor details
that will follow shortly under cleanup/testing/etc.
Note that this PR builds on top of PR #844 (and includes feedback from it), which is roughly the first half of the commits; I'll close #844. It also includes the requested shared_ptr -> regular pointer change requested in #872 (that didn't get included because of time constraints).