Fix: Use correct profile context for ledger signing during revocation setup (#3624)

[MonolithicMonk]

Solution:

• Added profile parameter and conditional context usage to relevant methods in acapy_agent/ledger/indy_vdr.py.
• Updated calls in acapy_agent/anoncreds/default/legacy_indy/registry.py to pass the profile parameter.
• Added new tests in acapy_agent/ledger/tests/test_indy_vdr.py and acapy_agent/anoncreds/default/legacy_indy/tests/test_registry.py specifically verifying the correct propagation and usage of the passed profile parameter.

Testing:

• Existing ledger and registry tests remain unchanged and pass, verifying backward compatibility / default behavior.
• New tests added to specifically validate that the explicit profile parameter is correctly passed through the registry and ledger layers and used for signing/TAA context within IndyVdrLedger._submit and related methods.

Considerations:

• This solution directly fixes the identified failure point by ensuring the correct context is available for critical operations.
• The underlying behavior of MultiIndyVDRLedgerManager returning potentially admin-bound ledger instances remains unchanged. A future refactor of ledger instance management/scoping might provide a more fundamental solution, but is outside the scope of this immediate fix.

Resolves #3624

[dbluhm]

You've done a ton of investigation and debugging -- thank you!

I'd like to understand more of what's going on here.

Passing a profile to a ledger method is a bit odd; Ledger instances are obtained through the injection context where a provider is bound on profile creation:

acapy/acapy_agent/askar/profile.py

Lines 139 to 156 in 8a83571

	injector.bind_provider(
	BaseLedger,
	ClassProvider(
	IndyVdrLedger,
	IndyVdrLedgerPool(
	write_ledger_config.get("pool_name")
	or write_ledger_config.get("id"),
	keepalive=write_ledger_config.get("keepalive"),
	cache=cache,
	genesis_transactions=write_ledger_config.get(
	"genesis_transactions"
	),
	read_only=write_ledger_config.get("read_only"),
	socks_proxy=write_ledger_config.get("socks_proxy"),
	),
	ref(self),
	),
	)

The ledger should only ever interact with this profile that it's instantiated with. Passing different profiles to a method is working against the patterns and assumptions baked into ACA-Py so I think we need to find another solution.

[ff137]

PS: sign commits with DCO, can be done with git rebase -i HEAD~7 and reword commits

[MonolithicMonk]

You've done a ton of investigation and debugging -- thank you!

I'd like to understand more of what's going on here.

Passing a profile to a ledger method is a bit odd; Ledger instances are obtained through the injection context where a provider is bound on profile creation:

acapy/acapy_agent/askar/profile.py

Lines 139 to 156 in 8a83571

	injector.bind_provider(
	BaseLedger,
	ClassProvider(
	IndyVdrLedger,
	IndyVdrLedgerPool(
	write_ledger_config.get("pool_name")
	or write_ledger_config.get("id"),
	keepalive=write_ledger_config.get("keepalive"),
	cache=cache,
	genesis_transactions=write_ledger_config.get(
	"genesis_transactions"
	),
	read_only=write_ledger_config.get("read_only"),
	socks_proxy=write_ledger_config.get("socks_proxy"),
	),
	ref(self),
	),
	)

The ledger should only ever interact with this profile that it's instantiated with. Passing different profiles to a method is working against the patterns and assumptions baked into ACA-Py so I think we need to find another solution.

@dbluhm , thanks for the feedback and the reference to the intended injection pattern. I completely agree that, ideally, the BaseLedger instance obtained via injection should inherently have the correct profile context it was created with, and we shouldn't need to pass the profile around explicitly to its methods.

However, the extensive debugging undertaken for this issue (partially documented in the issue comments and detailed in related discussions) revealed a breakdown in this pattern specifically within the asynchronous revocation setup flow following credential definition creation with endorsement enabled.

Here's a summary of the findings that led to the current solution:

Correct Initial Context: The process starts correctly. The initial API request, the event handler (on_rev_reg_def), and the subsequent service calls (AnonCredsRevocation, AnonCredsRegistry, LegacyIndyRegistry._revoc_reg_entry_with_fix) all demonstrably operate with the correct tenant profile context.

The Problem Point: get_ledger_for_identifier: The critical failure point occurred when retrieving the ledger instance within _revoc_reg_entry_with_fix:

profile prior to here is the correct tenant profile

acapy/acapy_agent/anoncreds/default/legacy_indy/registry.py

Lines 806 to 815 in 80d797c

	"""Send a revocation registry entry to the ledger with fixes if needed."""
	multitenant_mgr = profile.inject_or(BaseMultitenantManager)
	if multitenant_mgr:
	ledger_exec_inst = IndyLedgerRequestsExecutor(profile)
	else:
	ledger_exec_inst = profile.inject(IndyLedgerRequestsExecutor)
	_, ledger = await ledger_exec_inst.get_ledger_for_identifier(
	rev_list.rev_reg_def_id,
	txn_record_type=GET_REVOC_REG_ENTRY,
	)

From this point, 'ledger.profile' referenced the admin profile

Detailed logging confirmed that while ledger_exec_inst held the correct tenant profile, the call to get_ledger_for_identifier (which delegates to MultiIndyVDRLedgerManager) returned an IndyVdrLedger instance whose internal self.profile was the base/admin profile, not the tenant profile that initiated the request.

The underlying cause appears to be that the MultiIndyVDRLedgerManager holds or returns pre-initialized, shared IndyVdrLedger instances created at startup (bound to the admin profile), rather than providing profile-scoped instances.

Failure Mechanism: Consequently, when _revoc_reg_entry_with_fix called methods on the returned ledger object (specifically ledger.send_revoc_reg_entry which internally calls ledger._submit), those methods use self.profile (the incorrect admin profile) for wallet operations like signing. This resulted in the WalletNotFoundError because the tenant's signing key wasn't found in the admin wallet.

The Chosen Fix (Explicit Profile Passing): Given that the calling code (like _revoc_reg_entry_with_fix) did have the correct tenant profile object available, the most direct way to fix the immediate signing failure was to explicitly pass this correct profile down to the ledger methods (send_revoc_reg_entry, _submit, etc.). These methods were modified to prioritize the passed profile parameter over their potentially incorrect self.profile for wallet/TAA operations. This effectively bypasses the incorrectly profiled ledger instance returned by the manager for those specific sensitive operations.

While this approach deviates from the ideal injection pattern, it directly addresses the identified failure mode where the ledger instance provided by the current infrastructure lacked the necessary tenant context. Fixing the root cause within the MultiIndyVDRLedgerManager or the DI scoping for ledger instances would be a more fundamental solution but likely involves a larger refactor. This PR aims to restore the critical revocation functionality using the available correct profile context.

Happy to discuss alternative approaches or contribute to a deeper refactor if preferred.

[ff137]

Good work and test coverage 👍

[ff137]

Looks like the rebase was out of sync. Try git fetch upstream and git rebase upstream/main - hopefully fixes the extra changes

[jamshale]

I think this is ok for now. Thanks for figuring it out. Maybe we can just add some notes where the profile is added as a parameter that this was done because of an injection issue.

We should create an issue for looking into fixing the root of the injection problem.

[dbluhm]

I'm not able to reproduce; @jamshale I recommend holding off on merging until we know what's going on. My minimal example may not be perfectly capturing the issue we're seeking to fix here. Would really like to be able to see this in an isolated environment though; the fact that it works (according to my understanding of the setup as captured in the example) gives me pause.

[MonolithicMonk]

Can this be merged now pending a preferred resolution?

[jamshale]

So this is a temporary fix for the multi-ledger with a genesis-transactions-list configuration scenario? I'd like to see some more comments on the reason that the profile option is added to these functions so it doesn't get forgotten about forever.

I'm not opposed to allowing this for now when the root of the problem gets looked into. That's not an area I have looked at, so I'm not sure of the difficulty in finding a solution.

[MonolithicMonk]

Yes it is specific to the multi-ledger configuration scenario. Also are you referring to inline comments? Or does this pr + issue suffice?

[jamshale]

I was thinking some in code comments. We definitely want to make it clear to avoid this pattern and the PR and issues are easier to miss than having it recorded in the code.

[jamshale]

I see there's an effort going on to help find the root cause of that issue. Let's hold off on this for now.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud