chain: discover payer keys by the paired permission, not always active

[heifner]

The mismatch

authorization_manager has two places that reason about an explicit sysio.payer entry, and they disagreed.

Consensus (check_authorization) pairs the marker with any real permission the payer declares on the same action, then satisfies that entry:

if (auth.actor == payer && auth.permission != config::sysio_payer_name) {
   foundPayer = true;
   break;
}

Discovery (get_required_keys) hard-coded active:

if (declared_auth.permission == config::sysio_payer_name) {
   auto active_auth = permission_level{declared_auth.actor, config::active_name};
   SYS_ASSERT( checker.satisfied(active_auth), unsatisfied_authorization, … );
}

So a self-pay transaction paired under owner, or under a custom permission linked to the action, is accepted by consensus but rejected by discovery. Since /v1/chain/get_required_keys is what clio, kiod and every wallet call to learn which keys to sign with, such a transaction could not be signed through the standard path even though the chain would have taken it.

It stayed hidden because a freshly created account usually carries the same key on owner and active. It bites the accounts that separated them — which is the case the doc for this feature actively recommends.

The fix

The marker needs no check of its own. It is virtual: no permission_object backs it and it holds no keys. Consensus already requires it to be paired with a real permission, and that entry is checked on its own iteration of the same loop — so skipping the marker makes discovery agree with consensus by construction, rather than by keeping a second copy of the pairing rule in sync.

In the common case the reported key set is unchanged. Where it differs, it was previously wrong.

Not a consensus path. The only callers are the read-only RPC and producer_plugin signing its own votesnaphash transaction, so no protocol feature applies.

Test coverage

get_required_keys_explicit_payer_tests in unittests/api_tests.cpp, on payloadless::doit — chosen because it takes no arguments and asserts nothing about its authorization, so the authorization path is the only thing that can reject these transactions.

For each of the three pairings (active, owner, and a linked custom permission) it asserts both halves: the key set discovery reports, and that a transaction signed with exactly those keys is accepted. Agreement between the two is the property at issue — either half alone passed before the fix.

It also pins:

• discovery stays strict — candidates that cannot satisfy the paired permission still throw, including the neighbouring permissions in the hierarchy (active satisfies neither its parent owner nor its child custom), and an empty candidate set;
• the marker is attributed no key, via a two-action transaction pairing the same payer under two different permissions, whose result is exactly the union of those two.

Validation

• Guard verified: reinstating the @active check fails the owner and custom cases (2 of the discovery assertions, plus the strictness case whose message differs). The test fails without the fix and passes with it.
• Full suite: unit_test --sys-vm — 1516 test cases, *** No errors detected.

Note for reviewers

create_account already gives owner and active distinct keys, so the bug was reachable in the default tester and simply never exercised.

This blocks #583 (the ROA overview doc), which documents self-pay under owner/custom permissions as working end to end. That is true at consensus but was not discoverable before this change, so this should land first.

[huangminghuang]

[P2] Validate payer pairing before skipping the marker

/v1/chain/get_required_keys calls this function directly without running validate_referenced_accounts. With this unconditional continue, an action containing {alice@sysio.payer, bob@active} and only the candidate key for Bob now returns that key even though consensus rejects the transaction because Alice has no real authorization on the same action; a payer-only action can similarly return an empty key set. The base implementation rejected this candidate set while checking alice@active, so this broadens discovery success for transactions that no signature set can authorize.

Please preserve the index, uniqueness, and same-action/same-actor payer checks—ideally through a helper shared with consensus validation—while keeping the marker itself keyless, and add negative coverage for unpaired and cross-action payer declarations.

[heifner]

Fixed in 86e6523dda, though not with the shared helper -- deliberately.

The three structural rules are now re-checked in get_required_keys itself, mirroring check_authorization: position 0, at most one per action, and paired with a real permission from the same actor on the same action. Exception types and messages match the consensus validator so diagnostics agree.

I kept them duplicated rather than extracting a helper the consensus validators would also call. This function is not on the apply path, and refactoring check_authorization or validate_referenced_accounts to serve it would put a non-consensus caller in a position to change consensus behaviour -- not a trade worth making for a read-only endpoint. The comment records that reasoning so the duplication is not tidied away later.

Negative coverage added for each shape you named: payer paired with a different actor, payer with no real permission (the empty-set case), marker off index 0, two markers on one action, and cross-action pairing. Each asserts BOTH that discovery throws and that consensus rejects the same transaction, so the two cannot drift apart silently -- which is how this bug arose. I verified they fail against the unconditional continue: five failures, exactly the new cases.

One scope note. Two CFA cases are included, and an unpaired CFA marker is deliberately left unscreened: that rule lives in validate_referenced_accounts, not check_authorization, and pulling in one transaction-layer rule while ignoring account existence, permission existence and the rest would be arbitrary. The test asserts that asymmetry so a later change has to update the expectation rather than widen the function silently. Happy to close it if you would rather discovery mirror both layers.

Full sweep green: unit_test 1518, contracts_unit_test 655, plugin_test.

[huangminghuang]

The original finding is addressed. Please also refresh the PR description so it captures the complete current PR: it still says the marker needs no check and that simply skipping it ensures agreement, which contradicts the follow-up implementation. It also omits the new structural/CFA coverage and retains the old validation count.

[huangminghuang]

[P2] Track payer-marker presence separately

An empty actor is valid JSON input and decodes to account_name{}. For {empty@sysio.payer, bob@active}, assigning the marker actor leaves payer.empty() true, so Bob's key is accepted and this final pairing check is skipped. Both the base implementation and transaction validation reject that input. Track marker presence with an optional/boolean—or explicitly reject an empty actor—and add a regression case.

[heifner]

Fixed in 42a44037c6 -- and you are right, that was a real hole. account_name{} is what an empty string decodes to, so the marker assigned an empty actor, the sentinel still read as unset, and the pairing check was skipped. Now tracked with an explicit has_payer bool.

Worth noting where consensus sits on this: check_authorization uses the same payer.empty() idiom, but it is never exposed because validate_referenced_accounts rejects the non-existent actor first ("action's paying actor '' does not exist") and runs ahead of it on the apply path. That is precisely why the sentinel holds there and not here. I left the consensus copy alone.

Regression case added, and I verified it fails against the payer.empty() version and passes with the bool.

[huangminghuang]

[P2] Do not let irrelevant signatures satisfy rejection tests

This helper signs active, owner, and Bob keys for every malformed transaction, although owner is never declared. If the structural guards disappeared, check_authorization would still throw tx_irrelevant_sig, and the broad BOOST_CHECK_THROW(..., fc::exception) would remain green for the wrong reason. Sign only each case's declared real permissions and assert the expected structural exception/message.

[heifner]

Fixed in 42a44037c6. Fair hit -- that is the failure mode these tests exist to prevent, and I built it into the helper.

It now signs only the real permissions each case declares, derived from the transaction itself, and both halves assert a specific message instead of any fc::exception:

case	discovery	consensus
different actor	Payer authorization for	Payer 'payloadless' did not authorize this action
no real permission	Payer authorization for	same
empty actor	Payer authorization for	action's paying actor '' does not exist
off index 0	Explicit payer must be the first...	same
two markers	Multiple payers specified for action	action cannot have multiple payers
cross-action	Payer authorization for	Payer 'payloadless' did not authorize this action

So a case can no longer pass on tx_irrelevant_sig or any other unrelated rejection.

[huangminghuang]

Re-reviewed at 42a4403. The previously reported empty-actor and rejection-test issues are resolved; no remaining code findings.