Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add deprecate_deferred_transactions protocol feature #8697

Draft
wants to merge 46 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
03ad47d
Add `deprecate_deferred_transactions` protocol feature
Feb 24, 2020
6493043
Push changes; add exception
Feb 28, 2020
5a4c580
Pushing changes
Mar 5, 2020
738859e
Pushing changes
Mar 6, 2020
318c668
Pushing changes for the day
Mar 9, 2020
f5469bb
Update diff to locate bug
Mar 10, 2020
e0a19c3
Update diff to locate bug
Mar 10, 2020
fcf6d85
Update diff to locate bug
Mar 10, 2020
3a56333
Update diff to locate bug
Mar 10, 2020
d05174b
Update validating_tester
Mar 13, 2020
2c2fdc0
Add notes
Mar 13, 2020
d80ae1f
Pushing changes for debugging
Mar 13, 2020
9ecb930
`delay_tests` passing
Mar 16, 2020
4a855aa
`protocol_feature_tests` passing
Mar 16, 2020
cd6e39f
Trying a different strategy for testing proto feature activation
johndebord Mar 18, 2020
7c57d60
Checkpoint0
johndebord Mar 19, 2020
efd7640
Checkpoint1
johndebord Mar 19, 2020
45c073d
Checkpoint2: All tests passing
johndebord Mar 19, 2020
a99c7c1
Checkpoint3
johndebord Mar 20, 2020
5fa34b8
Testing git repo watcher script0
johndebord Mar 23, 2020
bbaf657
Cleanup
johndebord Mar 23, 2020
963cfbb
`protocol_feature_tests` passing again
johndebord Mar 23, 2020
01a34aa
All tests passing
johndebord Mar 23, 2020
6c9f0c1
Ready for review
johndebord Mar 23, 2020
c9e0439
Merge develop
johndebord Mar 24, 2020
1a4af45
Fix `currency_tests`
johndebord Mar 25, 2020
f6a3d85
Move `validating_tester` constructors to more readable location and f…
johndebord Mar 25, 2020
6fe80c4
eos-vm-oc tests fix attempt0
johndebord Mar 25, 2020
9cb5a21
Post code review0
johndebord Mar 26, 2020
b7d4e2c
Post code review1
johndebord Mar 27, 2020
310154a
Post code review2
johndebord Mar 30, 2020
91ec090
Post code review2
johndebord Mar 31, 2020
36bdc74
|
johndebord Apr 2, 2020
d619846
|
johndebord Apr 6, 2020
460d80b
Merge `develop`
johndebord Apr 6, 2020
084e7bf
|
johndebord Apr 6, 2020
ba86dd6
|
johndebord Apr 6, 2020
e671033
|
johndebord Apr 6, 2020
a89b9b7
Making sure both protocol features are checked consistently
johndebord Apr 9, 2020
fe76dc6
Tests working with both protocol features
johndebord Apr 9, 2020
418f91b
Merge `develop`
johndebord Apr 9, 2020
ee0efc0
Push weekend changes
johndebord Apr 13, 2020
1ef5c18
Pushing changes; generated transaction object table deletion
johndebord Apr 14, 2020
203e725
|
johndebord Apr 15, 2020
531f005
Failing on `light_validation_skip_cfa`; attempting to fix
johndebord Apr 15, 2020
ae25ee7
Fix `light_validation_skip_cfa` test
johndebord Apr 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 33 additions & 22 deletions libraries/chain/apply_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,13 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a
}

uint32_t trx_size = 0;

bool stop_deferred_transactions_activated = control.is_builtin_activated(builtin_protocol_feature_t::stop_deferred_transactions);

if ( stop_deferred_transactions_activated ) {
EOS_ASSERT( replace_existing, stop_deferred_tx, "you may only replace existing deferred transactions; not generate new ones" );
}

if ( auto ptr = db.find<generated_transaction_object,by_sender_id>(boost::make_tuple(receiver, sender_id)) ) {
EOS_ASSERT( replace_existing, deferred_tx_duplicate, "deferred transaction with the same sender_id and payer already exists" );

Expand All @@ -512,29 +519,33 @@ void apply_context::schedule_deferred_transaction( const uint128_t& sender_id, a

// Use remove and create rather than modify because mutating the trx_id field in a modifier is unsafe.
db.remove( *ptr );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a no-op, which doesn't seem consistent with the fact that send_deferred skips schedule_deferred_transaction entirely. Actually, I don't understand the motivation for allowing replace_existing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption, based off of the specification, is that there are two ways to enter deferred transaction logic:

  1. Through transaction_api::send_deferred.
  2. Through a user extending his/her code via a plugin and calling apply_context::schedule_deferred_transaction.

Therefore two individual (seemingly redundant) checks must be made. Am I wrong in this assumption?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think apply_context::schedule_deferred_transaction should ever be called outside of wasm. Also, the main point that I was trying to make was that the behavior is not the same in the two cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe they now have the same behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior is still different in the case of replace_existing = true. transaction_api::send_deferred short-circuits this case to a no-op, but apply_context::schedule_deferred_transaction handles it in a more complex way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what I was told to implement.

[XX] send_deferred
[XX] - If replace_existing == true in send_deferred; it shall be a noop

[XX] schedule_deferred_transaction
[XX] - If there is no transaction to replace: fail
[XX] - If there is a transaction to replace: continue to cancel the old one but do not schedule the new one

Hence, the assertion in apply_context::schedule_deferred_transaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the motivation for this difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation is unbeknownst to me. I just took it at face-value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat concerned that this may be a misunderstanding. The spec for schedule_deferred_transaction sounds to me like reasonable behavior for both of them. @arhag

db.create<generated_transaction_object>( [&]( auto& gtx ) {
gtx.trx_id = trx_id_for_new_obj;
gtx.sender = receiver;
gtx.sender_id = sender_id;
gtx.payer = payer;
gtx.published = control.pending_block_time();
gtx.delay_until = gtx.published + delay;
gtx.expiration = gtx.delay_until + fc::seconds(control.get_global_properties().configuration.deferred_trx_expiration_window);

trx_size = gtx.set( trx );
} );
if ( !stop_deferred_transactions_activated ) {
swatanabe-b1 marked this conversation as resolved.
Show resolved Hide resolved
db.create<generated_transaction_object>( [&]( auto& gtx ) {
gtx.trx_id = trx_id_for_new_obj;
gtx.sender = receiver;
gtx.sender_id = sender_id;
gtx.payer = payer;
gtx.published = control.pending_block_time();
gtx.delay_until = gtx.published + delay;
gtx.expiration = gtx.delay_until + fc::seconds(control.get_global_properties().configuration.deferred_trx_expiration_window);

trx_size = gtx.set( trx );
} );
}
} else {
db.create<generated_transaction_object>( [&]( auto& gtx ) {
gtx.trx_id = trx.id();
gtx.sender = receiver;
gtx.sender_id = sender_id;
gtx.payer = payer;
gtx.published = control.pending_block_time();
gtx.delay_until = gtx.published + delay;
gtx.expiration = gtx.delay_until + fc::seconds(control.get_global_properties().configuration.deferred_trx_expiration_window);

trx_size = gtx.set( trx );
} );
if ( !stop_deferred_transactions_activated ) {
db.create<generated_transaction_object>( [&]( auto& gtx ) {
gtx.trx_id = trx.id();
gtx.sender = receiver;
gtx.sender_id = sender_id;
gtx.payer = payer;
gtx.published = control.pending_block_time();
gtx.delay_until = gtx.published + delay;
gtx.expiration = gtx.delay_until + fc::seconds(control.get_global_properties().configuration.deferred_trx_expiration_window);

trx_size = gtx.set( trx );
} );
}
}

EOS_ASSERT( ram_restrictions_activated
Expand Down
10 changes: 9 additions & 1 deletion libraries/chain/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1244,7 +1244,10 @@ struct controller_impl {
trx->accepted = true;

transaction_trace_ptr trace;
if( gtrx.expiration < self.pending_block_time() ) {

bool stop_deferred_transactions_activated = self.is_builtin_activated(builtin_protocol_feature_t::stop_deferred_transactions);

if( gtrx.expiration < self.pending_block_time() || stop_deferred_transactions_activated ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to match the spec. "Only if a deferred transaction is expired can it be pushed" You're instead treating all transactions as if they were expired.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior now matches the spec; if the transaction has expired then the transaction shall expire. Then we check to see if builtin_protocol_feature_t::stop_deferred_transactions has been activated; if so, then the trace shall be returned, and no further operations are done on the transaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does producer_plugin know how to handle this outcome?

trace = std::make_shared<transaction_trace>();
trace->id = gtrx.trx_id;
trace->block_num = self.head_block_num() + 1;
Expand Down Expand Up @@ -1467,6 +1470,11 @@ struct controller_impl {

trx_context.delay = fc::seconds(trn.delay_sec);

if ( trx_context.delay.count() > 0 ) {
bool stop_deferred_transactions_activated = self.is_builtin_activated(builtin_protocol_feature_t::stop_deferred_transactions);
EOS_ASSERT( !stop_deferred_transactions_activated, stop_deferred_tx, "delay seconds must be 0" );
}

if( check_auth ) {
authorization.check_authorization(
trn.actions,
Expand Down
2 changes: 2 additions & 0 deletions libraries/chain/include/eosio/chain/exceptions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ namespace eosio { namespace chain {
3040018, "Transaction exceeded transient resource limit" )
FC_DECLARE_DERIVED_EXCEPTION( tx_prune_exception, transaction_exception,
3040019, "Prunable data not found" )
FC_DECLARE_DERIVED_EXCEPTION( stop_deferred_tx, transaction_exception,
3040019, "Deferred transactions have been stopped" )


FC_DECLARE_DERIVED_EXCEPTION( action_validate_exception, chain_exception,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ enum class builtin_protocol_feature_t : uint32_t {
webauthn_key,
wtmsig_block_signatures,
action_return_value,
stop_deferred_transactions
};

struct protocol_feature_subjective_restrictions {
Expand Down
12 changes: 12 additions & 0 deletions libraries/chain/protocol_feature_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,18 @@ may use a new `set_proposed_producers_ex` intrinsic to access extended features.
Builtin protocol feature: ACTION_RETURN_VALUE

Enables new `set_action_return_value` intrinsic which sets a value that is included in action_receipt.
*/
{}
} )

( builtin_protocol_feature_t::stop_deferred_transactions, builtin_protocol_feature_spec{
"DEPRECATE_DEFERRED_TRANSACTIONS",
fc::variant("56e2c91142e6ca088a2e982c51f94fc944ea9e478e4113fc46649d5e2c04b5b1").as<digest_type>(),
// SHA256 hash of the raw message below within the comment delimiters (do not modify message below).
/*
Builtin protocol feature: DEPRECATE_DEFERRED_TRANSACTIONS

Stops the ability to perform a deferred transaction.
*/
{}
} )
Expand Down
6 changes: 6 additions & 0 deletions libraries/chain/wasm_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1491,6 +1491,12 @@ class transaction_api : public context_aware_api {
}

void send_deferred( const uint128_t& sender_id, account_name payer, array_ptr<char> data, uint32_t data_len, uint32_t replace_existing) {
bool stop_deferred_transactions_activated = context.control.is_builtin_activated(builtin_protocol_feature_t::stop_deferred_transactions);

if ( stop_deferred_transactions_activated && replace_existing ) {
swatanabe-b1 marked this conversation as resolved.
Show resolved Hide resolved
return;
}

transaction trx;
fc::raw::unpack<transaction>(data, data_len, trx);
context.schedule_deferred_transaction(sender_id, payer, std::move(trx), replace_existing);
Expand Down
87 changes: 45 additions & 42 deletions libraries/testing/include/eosio/testing/tester.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ namespace eosio { namespace testing {
old_bios_only,
preactivate_feature_only,
preactivate_feature_and_new_bios,
full
complete
swatanabe-b1 marked this conversation as resolved.
Show resolved Hide resolved
};

std::vector<uint8_t> read_wasm( const char* fn );
Expand Down Expand Up @@ -146,13 +146,12 @@ namespace eosio { namespace testing {
typedef string action_result;

static const uint32_t DEFAULT_EXPIRATION_DELTA = 6;

static const uint32_t DEFAULT_BILLED_CPU_TIME_US = 2000;
static const fc::microseconds abi_serializer_max_time;

virtual ~base_tester() {};

void init(const setup_policy policy = setup_policy::full, db_read_mode read_mode = db_read_mode::SPECULATIVE);
void init(const setup_policy policy = setup_policy::complete, db_read_mode read_mode = db_read_mode::SPECULATIVE);
void init(controller::config config, const snapshot_reader_ptr& snapshot);
void init(controller::config config, const genesis_state& genesis);
void init(controller::config config);
Expand Down Expand Up @@ -379,7 +378,7 @@ namespace eosio { namespace testing {

void schedule_protocol_features_wo_preactivation(const vector<digest_type> feature_digests);
void preactivate_protocol_features(const vector<digest_type> feature_digests);
void preactivate_all_builtin_protocol_features();
void preactivate_builtin_protocol_features(const std::vector<builtin_protocol_feature_t>& ignored_features);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name will be confusing at the call site. If I see:

t.preactivate_builtin_protocol_features({wtmsig_block_signatures, replace_deferred});

I would assume that it meant the features to activate, not the features to exclude. I wouldn't even bother looking for the declaration because the meaning is "obvious."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to preactivate_selected_protocol_features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how the new name expresses the meaning of the function better than the old name. I think part of the problem is that the function's behavior is odd, which makes it hard to come up with a good name.


static genesis_state default_genesis() {
genesis_state genesis;
Expand Down Expand Up @@ -432,15 +431,16 @@ namespace eosio { namespace testing {
controller::config cfg;
map<transaction_id_type, transaction_receipt> chain_transactions;
map<account_name, block_id_type> last_produced_block;
unapplied_transaction_queue unapplied_transactions;
unapplied_transaction_queue unapplied_transactions;

public:
vector<builtin_protocol_feature_t> ignored_features;
vector<digest_type> protocol_features_to_be_activated_wo_preactivation;
};

class tester : public base_tester {
public:
tester(setup_policy policy = setup_policy::full, db_read_mode read_mode = db_read_mode::SPECULATIVE) {
tester(setup_policy policy = setup_policy::complete, db_read_mode read_mode = db_read_mode::SPECULATIVE) {
init(policy, read_mode);
}

Expand Down Expand Up @@ -516,49 +516,30 @@ namespace eosio { namespace testing {
wdump((e.to_detail_string()));
}
}
controller::config vcfg;

validating_tester(const flat_set<account_name>& trusted_producers = flat_set<account_name>()) {
auto def_conf = default_config(tempdir);

validating_tester(const ::boost::container::flat_set<::eosio::chain::account_name>& trusted_producers = ::boost::container::flat_set<::eosio::chain::account_name>(),
const ::std::vector<::eosio::chain::builtin_protocol_feature_t>& ignored_features = {},
const ::eosio::testing::setup_policy& policy = ::eosio::testing::setup_policy::complete) {
::std::pair<::eosio::chain::controller::config, ::eosio::chain::genesis_state> def_conf = default_config(tempdir);
::eosio::testing::base_tester::ignored_features = ignored_features;
swatanabe-b1 marked this conversation as resolved.
Show resolved Hide resolved
vcfg = def_conf.first;
config_validator(vcfg);
vcfg.trusted_producers = trusted_producers;

validating_node = create_validating_node(vcfg, def_conf.second, true);

init(def_conf.first, def_conf.second);
execute_setup_policy(setup_policy::full);
}

static void config_validator(controller::config& vcfg) {
FC_ASSERT( vcfg.blocks_dir.filename().generic_string() != "."
&& vcfg.state_dir.filename().generic_string() != ".", "invalid path names in controller::config" );

vcfg.blocks_dir = vcfg.blocks_dir.parent_path() / std::string("v_").append( vcfg.blocks_dir.filename().generic_string() );
vcfg.state_dir = vcfg.state_dir.parent_path() / std::string("v_").append( vcfg.state_dir.filename().generic_string() );

vcfg.contracts_console = false;
}

static unique_ptr<controller> create_validating_node(controller::config vcfg, const genesis_state& genesis, bool use_genesis) {
unique_ptr<controller> validating_node = std::make_unique<controller>(vcfg, make_protocol_feature_set(), genesis.compute_chain_id());
validating_node->add_indices();
if (use_genesis) {
validating_node->startup( [](){}, []() { return false; }, genesis );
}
else {
validating_node->startup( [](){}, []() { return false; } );
}
return validating_node;
execute_setup_policy(policy);
}

validating_tester(const fc::temp_directory& tempdir, bool use_genesis) {
// Delete this one after I fix the default one
validating_tester(const fc::temp_directory& tempdir, bool use_genesis, const std::vector<builtin_protocol_feature_t>& ignored_features = {}) {
base_tester::ignored_features = ignored_features;

auto def_conf = default_config(tempdir);
vcfg = def_conf.first;
config_validator(vcfg);

validating_node = create_validating_node(vcfg, def_conf.second, use_genesis);

if (use_genesis) {
init(def_conf.first, def_conf.second);
}
Expand All @@ -573,6 +554,7 @@ namespace eosio { namespace testing {
conf_edit(def_conf.first);
vcfg = def_conf.first;
config_validator(vcfg);

validating_node = create_validating_node(vcfg, def_conf.second, use_genesis);

if (use_genesis) {
Expand All @@ -583,6 +565,28 @@ namespace eosio { namespace testing {
}
}

static void config_validator(controller::config& vcfg) {
FC_ASSERT( vcfg.blocks_dir.filename().generic_string() != "."
&& vcfg.state_dir.filename().generic_string() != ".", "invalid path names in controller::config" );

vcfg.blocks_dir = vcfg.blocks_dir.parent_path() / std::string("v_").append( vcfg.blocks_dir.filename().generic_string() );
vcfg.state_dir = vcfg.state_dir.parent_path() / std::string("v_").append( vcfg.state_dir.filename().generic_string() );

vcfg.contracts_console = false;
}

static unique_ptr<controller> create_validating_node(controller::config vcfg, const genesis_state& genesis, bool use_genesis) {
unique_ptr<controller> validating_node = std::make_unique<controller>(vcfg, make_protocol_feature_set(), genesis.compute_chain_id());
validating_node->add_indices();
if (use_genesis) {
validating_node->startup( [](){}, []() { return false; }, genesis );
}
else {
validating_node->startup( [](){}, []() { return false; } );
}
return validating_node;
}

signed_block_ptr produce_block( fc::microseconds skip_time = fc::milliseconds(config::block_interval_ms) )override {
auto sb = _produce_block(skip_time, false);
auto bsf = validating_node->create_block_state_future( sb );
Expand Down Expand Up @@ -614,8 +618,6 @@ namespace eosio { namespace testing {
}

bool validate() {


auto hbh = control->head_block_state()->header;
auto vn_hbh = validating_node->head_block_state()->header;
bool ok = control->head_block_id() == validating_node->head_block_id() &&
Expand All @@ -633,9 +635,10 @@ namespace eosio { namespace testing {
return ok;
}

unique_ptr<controller> validating_node;
uint32_t num_blocks_to_producer_before_shutdown = 0;
bool skip_validate = false;
controller::config vcfg;
std::unique_ptr<controller> validating_node;
uint32_t num_blocks_to_producer_before_shutdown = 0;
bool skip_validate = false;
};

/**
Expand Down
35 changes: 29 additions & 6 deletions libraries/testing/tester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,11 @@ namespace eosio { namespace testing {
set_before_producer_authority_bios_contract();
break;
}
case setup_policy::full: {
case setup_policy::complete: {
schedule_preactivate_protocol_feature();
produce_block();
set_before_producer_authority_bios_contract();
preactivate_all_builtin_protocol_features();
preactivate_builtin_protocol_features(ignored_features);
produce_block();
set_bios_contract();
break;
Expand Down Expand Up @@ -1116,24 +1116,47 @@ namespace eosio { namespace testing {
}
}

void base_tester::preactivate_all_builtin_protocol_features() {
void base_tester::preactivate_builtin_protocol_features(const std::vector<builtin_protocol_feature_t>& ignored_features) {
const auto& pfm = control->get_protocol_feature_manager();
const auto& pfs = pfm.get_protocol_feature_set();
const auto current_block_num = control->head_block_num() + (control->is_building_block() ? 1 : 0);
const auto current_block_time = ( control->is_building_block() ? control->pending_block_time()
: control->head_block_time() + fc::milliseconds(config::block_interval_ms) );

set<digest_type> preactivation_set;
vector<digest_type> preactivations;
set<digest_type> preactivation_set;
vector<digest_type> preactivations;
std::vector<digest_type> ignored_digests;

ignored_digests.reserve(ignored_features.size());
for ( const auto& feature : ignored_features ) {
ignored_digests.push_back(*pfs.get_builtin_digest( feature ) );
}

std::function<void(const digest_type&)> add_digests =
[&pfm, &pfs, current_block_num, current_block_time, &preactivation_set, &preactivations, &add_digests]
[&ignored_digests, &pfm, &pfs, current_block_num, current_block_time, &preactivation_set, &preactivations, &add_digests]
( const digest_type& feature_digest ) {
const auto& pf = pfs.get_protocol_feature( feature_digest );
FC_ASSERT( pf.builtin_feature, "called add_digests on a non-builtin protocol feature" );
if( !pf.enabled || pf.earliest_allowed_activation_time > current_block_time
|| pfm.is_builtin_activated( *pf.builtin_feature, current_block_num ) ) return;

std::function<void(const digest_type&)> check_dependencies =
[&check_dependencies, &ignored_digests, &pf] ( const digest_type& feature ) {
const auto dependency_is_ignored = std::find( ignored_digests.cbegin(), ignored_digests.cend(), feature );
const auto dependency_has_dependency = std::find( pf.dependencies.cbegin(), pf.dependencies.cend(), feature );
if ( dependency_is_ignored != ignored_digests.cend() && dependency_has_dependency != pf.dependencies.cend() ) {
check_dependencies(*dependency_has_dependency);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this causes infinite recursion? *dependency_has_dependency should be equal to feature, right? I can't quite figure out what you're trying to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should now be checking protocol feature dependencies correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only handles the simple case of A depends on B, ignore B. Does not handle the possibility of chaining ignores: A -> B -> C -> D, ignore D, which needs to cause all of A, B, C, and D to be ignored.

Also I'm on the fence about whether it's better to ignore additional features or whether it's better to issue an error. On the one hand, it's bad if adding a new protocol feature breaks existing tests. On the other hand, silently disabling more protocol features than were given may be surprising. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely considered that case. But for the time being we only have the case of A depends on B.
Would it be best practice to account for future cases, such as chaining ignores? Or is the best practice only to account for what is currently in use?

And I definitely agree with you there on the last point. If it were up to me, I think tester is due for a re-write; to consider, from the ground up, an architecture where an arbitrary number of protocol features that may or may not depend on each other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best practice is to handle the public API of protocol features. Since arbitrary protocol feature dependencies are allowed, you should support them. Tester should not need any specific knowledge of what protocol features are actually defined. The existing code in tester to determine the activation order of protocol features is designed to handle arbitrary dependencies correctly, so I think it's reasonable for you to do so as well.

Now, with all of that having been said, being fully general is not always worth the effort. If you do decide to take shortcuts, you should definitely call it out in a comment, to help anyone who adds a new protocol feature and gets confused by mysterious failures.

} else {
FC_ASSERT( pf.builtin_feature, "ignoring a protocol feature that is a dependency for another protocol feature" );
swatanabe-b1 marked this conversation as resolved.
Show resolved Hide resolved
}
};

const auto digest_is_ignored = std::find(ignored_digests.cbegin(), ignored_digests.cend(), feature_digest);
swatanabe-b1 marked this conversation as resolved.
Show resolved Hide resolved
if( digest_is_ignored != ignored_digests.cend() ) {
check_dependencies(*digest_is_ignored);
return;
}

auto res = preactivation_set.emplace( feature_digest );
if( !res.second ) return;

Expand Down
Loading