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

Gh#4836 signals #4901

Merged
merged 10 commits into from
Aug 8, 2018
Merged

Gh#4836 signals #4901

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
292bbda
Add implicit and scheduled flags
heifner Jul 27, 2018
04f1cab
Check new implicit and scheduled flags instead of signatures for dete…
heifner Jul 27, 2018
43fad60
- emit accepted_transaction signal for implicit transaction onerror.
heifner Jul 27, 2018
2b19c74
Fix deferred transaction tests
heifner Jul 27, 2018
b6bcb08
Use explicit emit instead of scoped_exit. Attempting to fix unittests.
heifner Jul 28, 2018
d45faeb
Merge branch 'develop' into gh#4836-signals
heifner Aug 3, 2018
357ff35
Merge branch 'develop' into gh#4836-signals
heifner Aug 6, 2018
0c6edfa
Revert unneeded test changes
heifner Aug 7, 2018
31d8b7e
Assert not implicit/scheduled on push_transaction
heifner Aug 7, 2018
9550f29
Consistently emit before squash/undo
heifner Aug 7, 2018
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: 35 additions & 20 deletions libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,20 +222,20 @@ struct controller_impl {
std::cerr << std::setw(10) << next->block_num() << " of " << end->block_num() <<"\r";
}
}
std::cerr<< "\n";
ilog( "${n} blocks replayed", ("n", head->block_num) );

int rev = 0;
while( auto obj = reversible_blocks.find<reversible_block_object,by_num>(head->block_num+1) ) {
++rev;
self.push_block( obj->get_block(), controller::block_status::validated );
}

std::cerr<< "\n";
ilog( "${n} reversible blocks replayed", ("n",rev) );
auto end = fc::time_point::now();
ilog( "replayed ${n} blocks in ${duration} seconds, ${mspb} ms/block",
("n", head->block_num)("duration", (end-start).count()/1000000)
("mspb", ((end-start).count()/1000.0)/head->block_num) );
std::cerr<< "\n";
replaying = false;

} else if( !end ) {
Expand Down Expand Up @@ -495,6 +495,11 @@ struct controller_impl {
trx_context.billed_cpu_time_us, trace->net_usage );
fc::move_append( pending->_actions, move(trx_context.executed) );

auto onetrx = std::make_shared<transaction_metadata>( etrx );
onetrx->accepted = true;
onetrx->implicit = true;
emit( self.accepted_transaction, onetrx );

emit( self.applied_transaction, trace );

trx_context.squash();
Expand Down Expand Up @@ -559,19 +564,24 @@ struct controller_impl {
EOS_ASSERT( gtrx.delay_until <= self.pending_block_time(), transaction_exception, "this transaction isn't ready",
("gtrx.delay_until",gtrx.delay_until)("pbt",self.pending_block_time()) );

signed_transaction dtrx;
fc::raw::unpack(ds,static_cast<transaction&>(dtrx) );
transaction_metadata_ptr trx = std::make_shared<transaction_metadata>( dtrx );
trx->accepted = true;
trx->scheduled = true;

transaction_trace_ptr trace;
if( gtrx.expiration < self.pending_block_time() ) {
auto trace = std::make_shared<transaction_trace>();
trace = std::make_shared<transaction_trace>();
trace->id = gtrx.trx_id;
trace->scheduled = false;
trace->scheduled = true;
trace->receipt = push_receipt( gtrx.trx_id, transaction_receipt::expired, billed_cpu_time_us, 0 ); // expire the transaction
undo_session.squash();
emit( self.accepted_transaction, trx );
emit( self.applied_transaction, trace );
return trace;
}

signed_transaction dtrx;
fc::raw::unpack(ds,static_cast<transaction&>(dtrx) );

auto reset_in_trx_requiring_checks = fc::make_scoped_exit([old_value=in_trx_requiring_checks,this](){
in_trx_requiring_checks = old_value;
});
Expand All @@ -584,7 +594,7 @@ struct controller_impl {
trx_context.deadline = deadline;
trx_context.explicit_billed_cpu_time = explicit_billed_cpu_time;
trx_context.billed_cpu_time_us = billed_cpu_time_us;
transaction_trace_ptr trace = trx_context.trace;
trace = trx_context.trace;
try {
trx_context.init_for_deferred_trx( gtrx.published );
trx_context.exec();
Expand All @@ -599,11 +609,13 @@ struct controller_impl {

fc::move_append( pending->_actions, move(trx_context.executed) );

emit( self.applied_transaction, trace );

trx_context.squash();
undo_session.squash();

restore.cancel();

emit( self.accepted_transaction, trx );
emit( self.applied_transaction, trace );
return trace;
} catch( const fc::exception& e ) {
cpu_time_to_bill_us = trx_context.update_billed_cpu_time( fc::time_point::now() );
Expand All @@ -623,6 +635,8 @@ struct controller_impl {
trace = error_trace;
if( !trace->except_ptr ) {
undo_session.squash();
emit( self.accepted_transaction, trx );
emit( self.applied_transaction, trace );
Copy link
Contributor

Choose a reason for hiding this comment

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

apply_onerror would already emit an accepted_transaction signal for onerror transaction followed by an applied_transaction signal for the onerror transaction's trace (but without the inner deferred transaction trace).

These two lines would mean emitting the signals again in the case of soft_fail. Except that the accepted_transaction signal is emitted with the original transaction (not the on error) and the applied_transaction is emitted with the finalized, correct trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to me the correct thing here is to not emit in apply_onerror and instead handle all the exit conditions in push_scheduled_transaction.

return trace;
}
trace->elapsed = fc::time_point::now() - trx_context.start;
Expand All @@ -648,10 +662,11 @@ struct controller_impl {
block_timestamp_type(self.pending_block_time()).slot ); // Should never fail

trace->receipt = push_receipt(gtrx.trx_id, transaction_receipt::hard_fail, cpu_time_to_bill_us, 0);
emit( self.applied_transaction, trace );
undo_session.squash();
}

emit( self.accepted_transaction, trx );
emit( self.applied_transaction, trace );
return trace;
} FC_CAPTURE_AND_RETHROW() } /// push_scheduled_transaction

Expand Down Expand Up @@ -679,7 +694,6 @@ struct controller_impl {
*/
transaction_trace_ptr push_transaction( const transaction_metadata_ptr& trx,
fc::time_point deadline,
bool implicit,
uint32_t billed_cpu_time_us,
bool explicit_billed_cpu_time = false )
{
Expand All @@ -696,7 +710,7 @@ struct controller_impl {
trx_context.billed_cpu_time_us = billed_cpu_time_us;
trace = trx_context.trace;
try {
if( implicit ) {
if( trx->implicit ) {
trx_context.init_for_implicit_trx();
trx_context.can_subjectively_fail = false;
} else {
Expand All @@ -712,7 +726,7 @@ struct controller_impl {

trx_context.delay = fc::seconds(trx->trx.delay_sec);

if( !self.skip_auth_check() && !implicit ) {
if( !self.skip_auth_check() && !trx->implicit ) {
authorization.check_authorization(
trx->trx.actions,
trx->recover_keys( chain_id ),
Expand All @@ -729,7 +743,7 @@ struct controller_impl {

auto restore = make_block_restore_point();

if (!implicit) {
if (!trx->implicit) {
transaction_receipt::status_enum s = (trx_context.delay == fc::seconds(0))
? transaction_receipt::executed
: transaction_receipt::delayed;
Expand All @@ -747,8 +761,8 @@ struct controller_impl {

// call the accept signal but only once for this transaction
if (!trx->accepted) {
emit( self.accepted_transaction, trx);
trx->accepted = true;
emit( self.accepted_transaction, trx);
}

emit(self.applied_transaction, trace);
Copy link
Contributor

Choose a reason for hiding this comment

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

We emit the accepted_transaction and applied_transaction signals in push_transaction prior to squashing the undo sessions and canceling the scoped_exit to restore the block receipts, but we emit those signals after squashing the undo sessions in push_scheduled_transaction. It should be consistent.

The question is which one is more appropriate? You seem to have intentionally changed the point at which the existing signals were emitted to happen after. Was there a reason for this?

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 reason was my first stab at this I used a scoped exit to call the emits. However, that failed on gcc. I think I will go with the previous emit before squash/undo.

Expand All @@ -762,7 +776,7 @@ struct controller_impl {
trx_context.squash();
}

if (!implicit) {
if (!trx->implicit) {
unapplied_transactions.erase( trx->signed_id );
}
return trace;
Expand Down Expand Up @@ -827,11 +841,12 @@ struct controller_impl {

try {
auto onbtrx = std::make_shared<transaction_metadata>( get_on_block_transaction() );
onbtrx->implicit = true;
auto reset_in_trx_requiring_checks = fc::make_scoped_exit([old_value=in_trx_requiring_checks,this](){
in_trx_requiring_checks = old_value;
});
in_trx_requiring_checks = true;
push_transaction( onbtrx, fc::time_point::maximum(), true, self.get_global_properties().configuration.min_transaction_cpu_usage, true );
push_transaction( onbtrx, fc::time_point::maximum(), self.get_global_properties().configuration.min_transaction_cpu_usage, true );
} catch( const boost::interprocess::bad_alloc& e ) {
elog( "on block transaction failed due to a bad allocation" );
throw;
Expand Down Expand Up @@ -870,7 +885,7 @@ struct controller_impl {
if( receipt.trx.contains<packed_transaction>() ) {
auto& pt = receipt.trx.get<packed_transaction>();
auto mtrx = std::make_shared<transaction_metadata>(pt);
trace = push_transaction( mtrx, fc::time_point::maximum(), false, receipt.cpu_usage_us, true );
trace = push_transaction( mtrx, fc::time_point::maximum(), receipt.cpu_usage_us, true );
} else if( receipt.trx.contains<transaction_id_type>() ) {
trace = push_scheduled_transaction( receipt.trx.get<transaction_id_type>(), fc::time_point::maximum(), receipt.cpu_usage_us, true );
} else {
Expand Down Expand Up @@ -1332,7 +1347,7 @@ void controller::push_confirmation( const header_confirmation& c ) {

transaction_trace_ptr controller::push_transaction( const transaction_metadata_ptr& trx, fc::time_point deadline, uint32_t billed_cpu_time_us ) {
validate_db_available_size();
return my->push_transaction(trx, deadline, false, billed_cpu_time_us, billed_cpu_time_us > 0 );
return my->push_transaction(trx, deadline, billed_cpu_time_us, billed_cpu_time_us > 0 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert that trx->implicit is false so that it does not become possible for outside callers to push a transaction that is treated as an implicit transaction by controller.

}

transaction_trace_ptr controller::push_scheduled_transaction( const transaction_id_type& trxid, fc::time_point deadline, uint32_t billed_cpu_time_us )
Expand Down
6 changes: 4 additions & 2 deletions libraries/chain/include/eosio/chain/transaction_metadata.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,17 @@ class transaction_metadata {
packed_transaction packed_trx;
optional<pair<chain_id_type, flat_set<public_key_type>>> signing_keys;
bool accepted = false;
bool implicit = false;
bool scheduled = false;

transaction_metadata( const signed_transaction& t, packed_transaction::compression_type c = packed_transaction::none )
explicit transaction_metadata( const signed_transaction& t, packed_transaction::compression_type c = packed_transaction::none )
:trx(t),packed_trx(t, c) {
id = trx.id();
//raw_packed = fc::raw::pack( static_cast<const transaction&>(trx) );
signed_id = digest_type::hash(packed_trx);
}

transaction_metadata( const packed_transaction& ptrx )
explicit transaction_metadata( const packed_transaction& ptrx )
:trx( ptrx.get_signed_transaction() ), packed_trx(ptrx) {
id = trx.id();
//raw_packed = fc::raw::pack( static_cast<const transaction&>(trx) );
Expand Down
2 changes: 1 addition & 1 deletion plugins/bnet_plugin/bnet_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,7 +1190,7 @@ namespace eosio {
}

void on_accepted_transaction( transaction_metadata_ptr trx ) {
if( trx->trx.signatures.size() == 0 ) return;
if( trx->implicit || trx->scheduled ) return;
for_each_session( [trx]( auto ses ){ ses->on_accepted_transaction( trx ); } );
}

Expand Down
10 changes: 5 additions & 5 deletions unittests/api_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,10 +1016,10 @@ BOOST_FIXTURE_TEST_CASE(deferred_transaction_tests, TESTER) { try {
auto c = control->applied_transaction.connect([&]( const transaction_trace_ptr& t) { if (t->scheduled) { trace = t; } } );
CALL_TEST_FUNCTION(*this, "test_transaction", "send_deferred_transaction", {} );
BOOST_CHECK(!trace);
produce_block( fc::seconds(2) );
produce_block( fc::seconds(3) );

//check that it gets executed afterwards
BOOST_CHECK(trace);
BOOST_REQUIRE(trace);

//confirm printed message
BOOST_TEST(!trace->action_traces.empty());
Expand All @@ -1045,7 +1045,7 @@ BOOST_FIXTURE_TEST_CASE(deferred_transaction_tests, TESTER) { try {
control->push_scheduled_transaction(trx, fc::time_point::maximum());
}
BOOST_CHECK_EQUAL(1, count);
BOOST_CHECK(trace);
BOOST_REQUIRE(trace);
BOOST_CHECK_EQUAL( 1, trace->action_traces.size() );
c.disconnect();
}
Expand Down Expand Up @@ -1084,7 +1084,7 @@ BOOST_FIXTURE_TEST_CASE(deferred_transaction_tests, TESTER) { try {
auto c = control->applied_transaction.connect([&]( const transaction_trace_ptr& t) { if (t && t->scheduled) { trace = t; } } );
CALL_TEST_FUNCTION(*this, "test_transaction", "send_deferred_transaction", {});
CALL_TEST_FUNCTION(*this, "test_transaction", "cancel_deferred_transaction_success", {});
produce_block( fc::seconds(2) );
produce_block( fc::seconds(3) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not necessary, I made it while debugging another issue.

BOOST_CHECK(!trace);
c.disconnect();
}
Expand All @@ -1098,7 +1098,7 @@ BOOST_FIXTURE_TEST_CASE(deferred_transaction_tests, TESTER) { try {

produce_blocks(10);

{
{
// Trigger a tx which in turn sends a deferred tx with payer != receiver
// Payer is alice in this case, this tx should fail since we don't have the authorization of alice
dtt_action dtt_act1;
Expand Down