Make getRecentPerformanceSamples() return the number of vote and non-vote transactions

[steviez]

Problem

Currently, the getRecentPerformanceSamples() RPC method allows a user a query the number of transactions over some time window. The returned data contains the total number of transactions, but doesn't provide any insight into the number of vote vs. non-vote transactions. It would be nice for this method to return the number of votes and non-votes to have more granular data about the cluster.

Proposed Solution

Supposing that we want to we want to adjust the return type of getRecentPerformanceSamples() as suggested above, a couple things need to happen:

• Currently, SamplePerformanceService pushes PerfSample structs into the Blockstore. The number of transactions is queried from a Bank in this service; we'll need to store / be able to query the number of votes and non-votes in Bank.
• Adding extra field(s) to PerfSample stored in the Blockstore will be a breaking change if we store these items in the same column. Something will need to be done to address the incompatibility, such as adding a custom deserializer to the struct, such that both old and new instances of the struct in the Blockstore can be read out.
• Update the existing RPC function to return the new type (and whatever is entailed with making a change to this API).
  • Per some discussion on Discord, adding new fields is alright as the expectation is that older clients will ignore unknown fields if they get a response from server with newer version

[steviez]

Some more / context discussion in Discord:
https://discord.com/channels/428295358100013066/488758828330909706/1050539084583546880

[ngundotra]

Additional context: https://discord.com/channels/428295358100013066/531947850007969828/1052818896660471879

[CriesofCarrots]

@ilya-bobyr , I'm probably a good person to help review work on this

[CriesofCarrots]

Also, consider adding a minimumContextSlot input parameter to this method (https://discord.com/channels/428295358100013066/673718028323782674/1052811908845146162)

[0xksure]

Have anyone started on a branch? Would be very interested in tagging along on this.

[CriesofCarrots]

In consideration of the new Bank fields, we have another use-case that would benefit from the ability to query the total number of committed transactions in Bank

[ilya-bobyr]

Here is a change that address this request: #29353
Or, at least, it is a first step :)

The PR description explains the details. One difference with what was suggested in the issue is that instead of trying to deserialize two different versions of the stats, I've put new stats into a new column. It seems both better from the maintenance standpoint, and was actually much easier to implement.

I could not find a way to support deserialization based on the total object size when working with the serde/bincode combination. And rocksdb interaction layer has a deep assumption that values stored in columns can be deserialized with serde/bincode.

Making a decision about the data structure format based on the binary size also seems fragile. I think, there is a good chance more fields will be added into the performance stats set, and the need to count the number of committed transactions seems to be supporting this assumption.

@CriesofCarrots I could probably help with the other use case as well, now that I've spent some time learning this part of the codebase. Or do you want to combine these changes?

I was also wondering if it would make sense to add the non-vote transaction count to the explorer UI?
Should it be a separate change? Do we care about this stat in there?

@0xksure Check the linked PR. It is a single commit, not a branch, as it seems to be a rather tightly connected change.

[ilya-bobyr]

Another thing I'm still not sure about is the quality of the test coverage.

I think, I could add a local cluster test that would run a cluster with a single transaction, and then check that one transaction is reported in the cluster performance stats. One potential problem here is that the performance metrics seems to be collected based on a 30 seconds intervals. It would be non-optimal for a test to have to run for so long.

I also wonder if I should add a unit test or even several into runtime/src/bank.rs. The functionality I've added is similar to the existing stats. And I have updated all the unit tests in there to check the new stat. But, there could be some use cases that would not be covered by the existing tests.

[steviez]

I was also wondering if it would make sense to add the non-vote transaction count to the explorer UI?

Yep, being able to display non-vote tx count in explorer was the main motivation for this item 😄.

Should it (explorer update) be a separate change?

Yes, we should update explorer in a separate PR. I think solana-foundation/explorer#211 is tracking update for the explorer, and @ngundotra may have already looked at what would be needed once the updated RPC method has landed,

One difference with what was suggested in the issue ...

Fair enough - the issue writeup was meant as a primer to get started. I think we can discuss the merits of implementation details in the PR

[0xksure]

Yes, we should update explorer in a separate PR. I think solana-foundation/explorer#211 is tracking update for the explorer, and @ngundotra may have already looked at what would be needed once the updated RPC method has landed

I guess an update to web3 is necessary as well. From what I can tell it's enough to update the PerfSampleResult schema and the PerfSample type

[ngundotra]

@steviez how does the release schedule work for RPC updates like this? I'll have to update

solana/web3.js/src/connection.ts

Lines 4356 to 4372 in 926debd

	async getRecentPerformanceSamples(
	limit?: number,
	): Promise<Array<PerfSample>> {
	const unsafeRes = await this._rpcRequest(
	'getRecentPerformanceSamples',
	limit ? [limit] : [],
	);
	const res = create(unsafeRes, GetRecentPerformanceSamplesRpcResult);
	if ('error' in res) {
	throw new SolanaJSONRPCError(
	res.error,
	'failed to get recent performance samples',
	);
	}
	return res.result;
	}

before getting this into the Explorer UI

[steviez]

how does the release schedule work for RPC updates like this?

Recall that RPC nodes are running the same client as validators. Additionally, RPC operators typically lag behind the latest suggested version by a couple of releases for any given cluster (for the sake of stability).

MNB should be pushing towards v1.14 in Jan; if we want to consume this ASAP in explorer, we'd probably need to backport the change to v1.14. That being said, we have been trying to clamp down the backport process to critical issues in favor of getting onto a schedule where we are regularly pushing out releases.

From a functionality standpoint, I'd say this change is not critical. But, given the optics that come with TPS, we might opt to backport so we can push this out sooner. A discussion to be had in the releng channel once this lands.

[ilya-bobyr]

In consideration of the new Bank fields, we have another use-case that would benefit from the ability to query the total number of committed transactions in Bank

Thinking a bit more about it. In bank.rs I see this:

        let CommitTransactionCounts {
            committed_transactions_count,
            committed_non_vote_transactions_count,
            committed_with_failure_result_count,
            signature_count,
        } = counts;

        let tx_count = if self.bank_tranaction_count_fix_enabled() {
            committed_transactions_count
        } else {
            committed_transactions_count.saturating_sub(committed_with_failure_result_count)
        };

        self.increment_transaction_count(tx_count);

So, depending on the bank_tranaction_count_fix, numTransactions in the RPC response is either the total number of committed transactions, or the number of transactions that committed with a successful result.

I can easily add, for example, numCommitedTransactions to record the total number of committed transactions.
Unless someone has a different suggestion on the details of this.

As it would not require a change in the stat storage, it could also be a separate change.
But as they look close enough, I can combine them in a single branch.
Would it save effort in the release process?

[CriesofCarrots]

Hey @ilya-bobyr and all. Sorry, I missed the latest discussion here before commenting on the PR. I think it will be easier to talk about Bank and Blockstore changes separately, and we can do that on the separate PRs I've suggested over there. Wdyt?

I could not find a way to support deserialization based on the total object size when working with the serde/bincode combination. And rocksdb interaction layer has a deep assumption that values stored in columns can be deserialized with serde/bincode.

Making a decision about the data structure format based on the binary size also seems fragile. I think, there is a good chance more fields will be added into the performance stats set, and the need to count the number of committed transactions seems to be supporting this assumption.

Bincode can be instructed to fail if the data size is not correct, so it would be straightforward to attempt deserialization into each format in turn.
However, since you mention that there may be more fields added in the future, I now think that the best approach is probably to make the new data struct use protobuf serde. We have supporting apis already, like get_protobuf_or_bincode(). The Rewards and TransactionStatus columns use this approach, so their history will probably be instructive.

But anyway, we can discuss this more on the blockstore PR.

As it would not require a change in the stat storage, it could also be a separate change.
But as they look close enough, I can combine them in a single branch.
Would it save effort in the release process?

Generally, more smaller, separate PRs, the better. But as I mentioned on your existing PR, both the stat-storage change and the numCommittedTransactions change will require snapshot handling, and we can definitely lump them in the same new snapshot version. Let's sync up to see if it makes sense to approach this work in parallel :)

[ilya-bobyr]

I must admit, I completely missed this:

Also, consider adding a minimumContextSlot input parameter to this method (discord.com/channels/428295358100013066/673718028323782674/1052811908845146162)

Here is the referenced message:

I think the fix is probably to add min context slot to getRecentPerformancSamples so requests from nodes that are behind fail.

I do not understand what it means.
@CriesofCarrots Could you clarify what is the meaning of minimumContextSlot?

Should I move it into another issue or reopen this one?

[steviez]

Should I move it into another issue or reopen this one?

Let's leave this issue closed, I think you can either make a new issue or just a new PR.

Could you clarify what is the meaning of minimumContextSlot?

To start out, I would suggest picking another method from the API that has a minContextSlot arg, and trace that through code to see how it affects the results.

As to why we need it here, note that the methods you added record the number of transactions and slots processed in some amount of time. However, those performance samples are unaware of the state of the node in relation to the cluster. For example, suppose a node was just recently offline.

In order to catch up, by definition, the node will have to replay slots faster than the cluster is producing them. If we get some perf. samples from this node while it is catching up, the tx processed numbers will be deceptively high (because we report transactions-per-second, not transactions-per-slot), and not representative of what the cluster is actually doing in realtime.

Setting a minContextSlot could help us filter out samples from a catchup period