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

Build/core test fixes, incorporate LNS construct_tx revamp #939

Merged
merged 13 commits into from Dec 4, 2019

Conversation

@Doy-lee
Copy link
Collaborator

Doy-lee commented Nov 29, 2019

This also merges in a manually merged #913 because the staking changes here fix up some of the redundant-ness in loki_construct_tx_params (you can derive most of the current params by just sending in the hf_version and desired tx_type) and I didn't want to continue fixing around the old style params in core_tests to have it re-replaced again in LNS.

Core tests are all passing as of this commit.

  • Fix assert to use version_t::_count in service_node_list.cpp

  • Incoporate staking changes from LNS which revamp construct tx to
    derive more of the parameters from hf_version and type. This removes
    extraneous parameters that can be derived elsewhere.

    Also delegate setting loki_construct_tx_params into wallet2 atleast,
    so that callers into the wallet logic, like rpc server, simplewallet
    don't need bend backwards to get the HF version whereas the wallet has
    dedicated functions for determining the HF.

  • Revert double spend behaviour fixes core_test loki_core_test_deregister_on_split

  • Setting up tx.is_transfer() to encapsulate, normal transfers, staking transfers and name service transfers- so that we can apply normal TX validation rules on all those types of transactions.

  • Remove some params in tx_params as not necessary in construct_tx anymore

  • Describe how infinite staking works in code

  • Remove nettype from min tx version function. Just always enforce the lower version, no harm in doing so across all the nettypes.

Edit: Also merges in core tests from #909

@@ -2142,7 +2197,7 @@ namespace service_nodes
info.version = version_t::v3_quorumnet;
}
// Make sure we handled any future state version upgrades:
assert(info.version == static_cast<version_t>(static_cast<uint8_t>(version_t::count) - 1));
assert(info.version == static_cast<version_t>(static_cast<uint8_t>(version_t::_count) - 1));

This comment has been minimized.

Copy link
@jagerman

jagerman Dec 2, 2019

There's a tools::enum_count<version_t> to grab this now (which is why I unified them all to use _count), so this can just be:

static_cast<version_t>(tools::enum_count<version_t> - 1);

though it might be useful to add another version of this to common/util.h that returns the value before the _count, as it seems pretty common:

template <typename T>
constexpr T enum_top = static_cast<T>(enum_count<T> - 1);

then the above code can become just:

assert(info.version == enum_top<version_t>);

and I seem to recall there are a couple other places that could do the same. Edit: I had a look and couldn't find any, but I still think it's a nice little addition.

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Dec 2, 2019

Author Collaborator

Will update.


if (tx_params.burn_percent)
{
LOG_ERROR("cannot construct tx: internal error: burn percent must be converted to fixed burn amount in the wallet");
return false;
}

if (tx_params.burn_fixed && tx_params.hf_version <= cryptonote::network_version_13_enforce_checkpoints)

This comment has been minimized.

Copy link
@jagerman

jagerman Dec 2, 2019

Minor point: < version_14_blink_lns would seem slightly more obvious when reading the code than <= version_13_enforce_checkpoints (and agrees more with the english language description in the error message).

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Dec 2, 2019

Author Collaborator

Funny. I actually feel the opposite, I actually prefer seeing condition >= X+1 && condition <= X-1 in state machine-like checks. It indicates the exact machine state the branch would execute, I know < X executes, but what is the state at <= X-1 that we're disallowing.

Any who, I will update. Just a side comment.

This comment has been minimized.

Copy link
@jagerman

jagerman Dec 2, 2019

It wasn't so much the <= vs <, but rather the < blink versus <= something_else here where the intention is "must be blink". (I'd also be fine with: !(hf_version >= v14_blink) here).

@@ -2206,8 +2201,11 @@ namespace boost
x.rct_config = { use_bulletproofs ? rct::RangeProofBulletproof : rct::RangeProofBorromean, 0 };
return;
}
a & x.v4_allow_tx_types;

This comment has been minimized.

Copy link
@jagerman

jagerman Dec 2, 2019

Don't we still need the above fields present to properly deserialize tx data constructed by an older wallet? I think each could be guarded by a if (ver < 6) so that we don't put them in for v6, but I think when reading an existing one we still need to set t.tx_type appropriately when reading an older serialization version.

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Dec 2, 2019

Author Collaborator

This whole kinda thing is almost useless, given its TX construction data which only gets serialized if you're trying to offline sign a TX. You'd have to open a view only wallet, create the transaction on our v5 wallet. Then download v6 and try to sign the constructed transaction to no avail.

People aren't holding onto constructed transactions for later signing either because then you'd fail sanity checks if that constructed transaction was sitting in storage for a long time.

So I didn't give much consideration to writing it bulletproof. Though that selected_transfers should not have been deleted. I can re-do the necessary fields.

This comment has been minimized.

Copy link
@jagerman

jagerman Dec 2, 2019

Ah, okay, makes sense. I think it's fine to not support signing across mixed wallet versions.

loki_tx_params.tx_type = tx_type;
loki_tx_params.hf_version = *hf_version;
bool burning = loki_tx_params.burn_fixed || loki_tx_params.burn_percent;
THROW_WALLET_EXCEPTION_IF(burning && loki_tx_params.hf_version < HF_VERSION_FEE_BURNING, error::wallet_internal_error, "cannot construct transaction: cannot burn amounts under the current hard fork");

This comment has been minimized.

Copy link
@jagerman

jagerman Dec 2, 2019

I like the simplification overall, but I'm not so happy with this part of the change. It works here for blink because blink doesn't have any use for tx priorities, but this code was deliberately meant to support other future cases that will want to use burning that do have a use case for priorities (I had LNS registrations in mind here - where you want to burn some fixed fee, but might always want to submit at a higher priority to beat other TXes to getting mined). By putting the the burn amount into loki_tx_params and passing that in, as it is before this PR, you can use it to submit (for example) a .loki registration that has to burn 25 LOKI but still lets you choose your priority to try to get it mined onto the chain faster.

However, I also wasn't happy with the places I had to add code to pass this in, so I have an improvement that should keep both the calling simplification this PR adds without throwing away the flexibility for places that want/need that added flexibility. The idea is to use an implicit conversion constructor in loki_construct_tx_params like this:

loki_construct_tx_params() = default;
loki_construct_tx_params(cryptonote::txtype tx_type) : tx_type{tx_type} {}

and then keeping the last create_transactions_* argument as a const loki_construct_tx_params & (rather than a txtype). Then in most places (including the default argument!) you can still just pass in a txtype::standard, but more complicated cases that want something else can still build and pass a full loki_construct_tx_params.


Proof of concept showing how this works:

#include <iostream>

enum class TxType { standard, special, whatever };

struct construct_params {
    TxType tx_type = TxType::standard;
    int more_stuff = 0;

    construct_params() = default;
    construct_params(TxType t) : tx_type{t} {}
};

// "create_transactions_2":
void print_type(int a, int b, const construct_params &p = TxType::standard) {
    std::cout << "given params tx_type = " <<
        (p.tx_type == TxType::standard ? "standard" :
         p.tx_type == TxType::special ? "special" : "other") <<
        " with " << p.more_stuff << " more stuff\n";
}

int main() {
    print_type(1, 2, TxType::special);

    construct_params more_complex;
    more_complex.tx_type = TxType::whatever;
    more_complex.more_stuff = 42;
    print_type(3, 4, more_complex);

    print_type(5, 6);
}

prints:

given params tx_type = special with 0 more stuff
given params tx_type = other with 42 more stuff
given params tx_type = standard with 0 more stuff

This comment has been minimized.

Copy link
@Doy-lee

Doy-lee Dec 2, 2019

Author Collaborator

I understand. Thought I'd just add a txtype to construct_params for when LNS is added to the wallet.

The construct params has fields that the front ends (simplewallet, wallet lib interface, wallet rpc server) shouldn't have to care about filling out: the burn amounts and hf version. Those aren't user configurable and burns can be figured out by the txtype + priority so that was moved to happen at a more centralised location in wallet2.

@Doy-lee Doy-lee force-pushed the Doy-lee:BuildCoreTestFixes branch from 134020d to 165e212 Dec 2, 2019
@Doy-lee Doy-lee force-pushed the Doy-lee:BuildCoreTestFixes branch from 165e212 to 291ba11 Dec 4, 2019
@Doy-lee Doy-lee merged commit cf79843 into loki-project:dev Dec 4, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
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.