Postpone decision on delay_sec to assume for authorization checking within eosio.msig until transaction execution

[arhag]

Limitation of the current eosio.msig contract

Currently, within the eosio.msig contract, a particular value of delay_sec must be committed to when a user proposes a transaction. That value is used to determine which of the wait_weights should be considered satisfied with the permission authorities traversed when checking the authorizations on the transaction.

But this comes with a limitation. If there are multiple paths to satisfying the authorizations which assume different values of delay_sec, then the path to take must be committed to during proposal time. If approvers decide to take a different path, they are forced to replace the proposal with another one with a different value of delay_sec. Or alternatively, they could put competing (and naturally conflicting) proposals for the various paths that can be exercised, and then see which one gets the necessary approvals first.

The limitation is more clear with a concrete example. Consider a transaction requiring an authority that can be satisfied by one of two paths: 1. two accounts approve and the transaction can be executed with no delay; 2. one account approves and the transaction can be executed with a delay of 48 hours. The transaction proposal expects both approvers to be ready to review the msig and approve within 24 hours. So the proposer proposes a transaction with a delay_sec of 0. The first approver approves the proposal within 24 hours. But the second approver is not available and 48 hours passes since the time the first approval was made. At this point the proposer may wish that they had proposed with a delay_sec of 172800 (48 hours). If they had done so, they would be able to execute the proposed transaction without requiring the second approver. But to change it now, they would have to replace the proposal with a new one that sets the delay_sec of 172800 and wait again for the first approver to approve this second proposal as well.

Proposed enhancement to eosio.msig contract

The proposer should not need to commit to a particular value of delay_sec. Instead, a value for delay_sec can optionally be provided at execution time. And in fact, the propose action can assert that the delay_sec value within a proposed transaction is always zero.

The earliest_execution_time field in a proposal is no longer used. Instead, the time aspects of authorization checking are postponed until the exec call. The approve and unapprove action no longer need to make calls to check transaction authorizations.

The logic of authorization checking for the transaction becomes more sophisticated during the exec call. First, a new argument is needed to provide the assumed delay_sec to use when doing authorization checking; though it can default to 0. The assumed delay_sec filters out any approvals that are too recent when it comes to transaction authorization checking.

Details

propose action

Because the proposer no longer needs to commit to a particular value of delay_sec, the action should simply assert that the value of delay_sec in the transaction header of the proposed transaction is zero.

There is no more need for the earliest_execution_time field in the proposal table going forward. However, the field should not be removed because existing tables have a serialization expecting that field to exist. So the field will need to forever remain in its current place in the struct for the table named proposal to avoid a mistake in the future where a new binary extension is added to the struct for some future feature.

The propose action can continue to create the proposal table entry with the earliest_execution_time initialize to the empty optional. Going forward, new proposals will always have the earliest_execution_time remain as an empty optional.

approve action

Consider the following code:

reference-contracts/contracts/eosio.msig/src/eosio.msig.cpp

Lines 129 to 137 in 76197b4

	if( prop.earliest_exec_time.has_value() ) {
	if( !prop.earliest_exec_time->has_value() ) {
	auto table_op = [](auto&&, auto&&){};
	if( trx_is_authorized(get_approvals_and_adjust_table(get_self(), proposer, proposal_name, table_op), prop.packed_transaction) ) {
	proptable.modify( prop, proposer, [&]( auto& p ) {
	p.earliest_exec_time.emplace(time_point{ current_time_point() + eosio::seconds(trx_header.delay_sec.value)});
	});
	}
	}

Both the existing code and the code with the proposed changes in this issue would enter the if( prop.earliest_exec_time.has_value() ) if statement. However, the code within the if( !prop.earliest_exec_time->has_value() ) { if statement is no longer necessary with the proposed changes in this issue. The earliest_exec_time is only needed for the exec action, and with the changes discussed here, the exec action would have an alternative way of handling the time aspect of authorization checking. The exec action could still do the time check if earliest_exec_time->has_value() is true; but that condition would not relevant for any new proposals.

It may me desirable for the approve action to always ensure earliest_exec_time of the proposal is the empty optional, but this is not necessary.

unapprove action

Consider the following code:

reference-contracts/contracts/eosio.msig/src/eosio.msig.cpp

Lines 169 to 177 in 76197b4

	if( prop.earliest_exec_time.has_value() ) {
	if( prop.earliest_exec_time->has_value() ) {
	auto table_op = [](auto&&, auto&&){};
	if( !trx_is_authorized(get_approvals_and_adjust_table(get_self(), proposer, proposal_name, table_op), prop.packed_transaction) ) {
	proptable.modify( prop, proposer, [&]( auto& p ) {
	p.earliest_exec_time.emplace();
	});
	}
	}

Both the existing code and the code with the proposed changes in this issue would enter the if( prop.earliest_exec_time.has_value() ) if statement. However, the code within the if( prop.earliest_exec_time->has_value() ) { if statement is no longer necessary with the proposed changes in this issue for the same reasons discussed above for the approve action.

It may me desirable for the unapprove action to always ensure earliest_exec_time of the proposal is the empty optional, but this is not necessary.

exec action

An argument eosio::binary_extension<uint32_t> delay_sec (or alternatively eosio::binary_extension<std::optional<uint32_t>> delay_sec) should be added to the end of the existing exec action. The default value for delay_sec is 0. This value provided via delay_sec is used to subtract that number of seconds from the current time_point of the block and get the approval_threshold_time.

Then that time is used to filter the set of approvals on the proposal to only select for those approvals that have not been invalidated and have a time of approval less than or equal to the approval_threshold_time. Those filtered approvals are used to check if the proposed transaction is authorized, however the transaction authorization is done on a variation of the transaction in which the delay_sec field of the transaction header is replaced by the value provided in the delay_sec argument of the action. If that authorization check passes, then the exec action will execute the transaction by sending inline actions.

Note: For old proposals that use the old approval structure which does not have time, the action will assert if a non-zero value is provided in the delay_sec argument of the action. This is not a meaningful limitation since the existing contract already asserts if an old proposal has a proposed transaction with a non-zero value for delay_sec within its transaction header and asks the user to cancel the proposal and retry.