Skip to content

Solana plugin: blockhash handling + maintenance audit #881

Description

@jmoreira-valory

Problem

The open-aea-ledger-solana plugin has accumulated correctness and maintenance debt that should be addressed in a dedicated refactor. The dep bump in #(this PR) re-enables the plugin for downstream consumers (see valory-xyz/open-autonomy#2479) but intentionally preserves pre-existing behaviour to keep that change minimal and review-able. This issue tracks the items that should be cleaned up next.

Correctness: blockhash handling

The plugin caches a recent blockhash with a wall-clock TTL (10 seconds, both before and after #(this PR) — the upstream solana.blockhash.BlockhashCache being replaced was itself TTL-based). Solana blockhash validity is tied to block height, not time:

  • a blockhash expires when current_block_height > last_valid_block_height (~150 blocks, ~60–90s but variable with cluster load)
  • context.slot from get_latest_blockhash is the slot the RPC responded at, not an expiration bound
  • under load / async delays this yields intermittent Blockhash not found at submit time

Needed:

  • Replace the TTL cache with a height-based model:
    • fetch value.blockhash and value.last_valid_block_height from get_latest_blockhash()
    • on get, call get_block_height() and compare against last_valid_block_height — expire if exceeded
    • typed attribute access on the response (no to_json()json.loads(...) roundtrip)
  • OR (simpler, preferred for agents that submit infrequently) remove the cache entirely and fetch per-transaction. SolanaApi.latest_hash already does an unconditional RPC call on every access today, so the cache is mostly ornamental.
  • Drop the unused slot parameter from BlockhashCache.set() — it is stored but never read back.

Correctness: other pre-existing issues

  • _generate_tx_nonce calls self._api.BlockhashCache.get(), but BlockhashCache is an attribute of SolanaApi, not ._api (a SolanaApiClient — confirmed: hasattr(SolanaApiClient(...), 'BlockhashCache') is False). Every call raises AttributeError, is caught by the broad except Exception, and falls through to a fresh RPC. The cache has been ornamental in practice — removing it entirely in a follow-up changes nothing behaviourally. Correct reference: self.BlockhashCache.get().
  • Broad except Exception in _generate_tx_nonce swallows RPC failures, serialization errors, and logic bugs alike. Narrow to LookupError (cache miss) so real failures surface.
  • Missing null check on get_latest_blockhash().value — crashes with AttributeError if the RPC returns no value rather than raising.
  • File-handle leak in SolanaHelper.load_contract_interface: open(bytecode_path, "rb") without a with block. Add context manager + explicit encoding.
  • JSON roundtrip in _get_latest_hash and _generate_tx_nonce (result.value.to_json()json.loads(...)) — result.value.blockhash is already a typed string. Remove the roundtrip.

Naming / API clarity

  • The AEA base class Helper._generate_tx_nonce is chain-agnostic ("nonce" = replay-defeater). For Solana readers this is confusing — blockhashes are an anti-replay mechanism, distinct from Solana's actual durable-nonce accounts. Add a docstring clarifying what "nonce" means for this plugin, or introduce a Solana-specific name alongside the overriding method.
  • generate_tx_nonce(seller, client) (the @staticmethod sha256 variant on SolanaHelper) is literally unrelated to Solana — pure deterministic hash. Document or remove.

Dependency hygiene

  • The plugin is pinned tight at solana>=0.33.0,<0.34.0 in #(this PR). solana>=0.36 removed the solana.transaction module (replaced by solders.transaction) — adopting it requires refactoring all Transaction imports + call sites. Worth doing so we can track solana-py's current line.
  • anchorpy>=0.20.0,<0.21.0anchorpy 0.21+ requires solders>=0.21 + solana>=0.36 (the two bumps above). Group these.
  • CI coverage: the plugin has 33 tests but 9 are devnet-network-dependent (hit api.devnet.solana.com faucet) and are failing today due to airdrop rate-limit exhaustion. Either gate them behind a @pytest.mark.integration marker excluded from default runs, or stub the RPC boundary so unit tests remain deterministic.

Mypy / pylint baseline

Not blocking this follow-up, but for reference the plugin currently carries 51 mypy errors and ~20 pylint warnings that long predate any recent change. Worth a tracked clean-up pass.

Context

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions