ledger/blockstore: PerfSampleV2: num_non_vote_transactions

[ilya-bobyr]

Problem

As part of work on #29159 we need to store "non-vote transaction counts" in the blockstore.

Summary of Changes

Store non-vote transaction counts that will be recorded by the banks into the blockstore.

Part of work on #29159

[steviez]

Trying to be as friendly to future us as possible, I'm thinking about the following sequence which does come up:

1. Someone is running master or some branch that has this commit (ie inserting V2s)
2. They run into an issue unrelated to this change that causes them to want to bisect (with the same ledger, maybe via solana-ledger-tool verify)
3. This person at some point ends up trying to run a commit older than the one this PR will introduce
4. The older commit isn't aware of PerfSampleV1 & V2, only the original PerfSample struct which is 18 bytes (as is V1) and as such, will try to deserialize blockstore entries into the original struct

Given that we use the serialize / deserialize functions (and not the option), deserializing an instance of V2 (26 bytes) that was inserted in 1) above (ie with this commit) will not error in 4) as an original PerfSample, since the deserialize function allows trailing bytes by default (original has one less u64 so 8 trailing bytes).

Even though we won't get an error, the ordering will have changed so we'll be interpreting fields incorrectly. This can be avoided if we move the new field to the end of the struct. I realize this is pretty brittle, but it is better than doing nothing. All this being said, I struggle to see a scenario where we'd be debugging something of the sort and examining this column, but better to have one less possible foot-gun I guess.

Note: I'm pretty sure bincode serializes struct fields in the order specified, even if the compiler would want to rearrange things for better packing (ie multiple u16's in the same struct as a u64).

[steviez]

I was poking around bincode's GitHub and ended up deep enough down the rabbit hole to lead back to discussion on our GH in regards to a similar problem that this issue looks to solve. The below comment has a suggestion that might work nicely for us here (it ended up not working in the scenario where it was proposed, but it should work here):
#12494 (comment)

This is consistent with my last comment about making sure the new field is at the end of the struct. In any case, I think some of the commentary in referenced issue is relevant, and is worth consideration here.

[CriesofCarrots]

Just one syntax suggestion and tiny nit

[CriesofCarrots]

Caught one typo. Otherwise, looks good to me!

[steviez]

Just a couple of nitpicks / minor suggestions on my end, so I think good to go after those and Tyera's comments addressed

Pull request has been modified.

[steviez]

LGTM, looks like just one bad keystroke. Also, nice on getting rid of explicit drop()

        let (bank, highest_slot) = {
            let forks = bank_forks.read().unwrap();
            (forks.root_bank(), forks.highest_slot())
        };

[CriesofCarrots]

Also, nice on getting rid of explicit drop()

Fwiw, it would be nice if, in future, refactors like that code block were done in a separate commit from additions to make review a little easier. (But don't worry about changing this PR!)

[ilya-bobyr]

Also, nice on getting rid of explicit drop()

Fwiw, it would be nice if, in future, refactors like that code block were done in a separate commit from additions to make review a little easier. (But don't worry about changing this PR!)

This is where Gerrit would have been a much better tool ;)
Gerrit would allow me to easily split a change into a separate commit, and you would be able to review and approve it.
While the rest of the changes are sitting on top, still going through the review process.

With GitHub I need to juggle multiple branches, making sure they are in sync.
Lots of overhead...
Though, maybe, I just need to find a way to do it with GitHub =)

Pull request has been modified.

[steviez]

Also, nice on getting rid of explicit drop()

Fwiw, it would be nice if, in future, refactors like that code block were done in a separate commit from additions to make review a little easier. (But don't worry about changing this PR!)

Good point Tyera; I found the variables a little confusing so I made a request to change, but in general, agreed that it is better to break cleanup into its' own PR.

This is where Gerrit would have been a much better tool ;) Gerrit would allow me to easily split a change into a separate commit, and you would be able to review and approve it. While the rest of the changes are sitting on top, still going through the review process.

With GitHub I need to juggle multiple branches, making sure they are in sync. Lots of overhead... Though, maybe, I just need to find a way to do it with GitHub =)

Yeah, unfortunately not a better way to do it with GitHub that I'm aware of. When I see a small cleanup task that can be carved out, I usually like to have to have it in master before my original PR (personal preference but extra overhead). But yeah, make a second branch / second PR, get that submitted and then rebase original branch on top.

[CriesofCarrots]

Though, maybe, I just need to find a way to do it with GitHub =)

Yeah, I don't know of any way to use git to assign parts of the current changeset to separate commits, unless they are in separate files. I usually use interactive rebase and recreate the first sub-changeset. Works okay.

[ilya-bobyr]

This is where Gerrit would have been a much better tool ;) Gerrit would allow me to easily split a change into a separate commit, and you would be able to review and approve it. While the rest of the changes are sitting on top, still going through the review process.
With GitHub I need to juggle multiple branches, making sure they are in sync. Lots of overhead... Though, maybe, I just need to find a way to do it with GitHub =)

Yeah, unfortunately not a better way to do it with GitHub that I'm aware of.

Sad :(

When I see a small cleanup task that can be carved out, I usually like to have to have it in master before my original PR (personal preference but extra overhead). But yeah, make a second branch / second PR, get that submitted and then rebase original branch on top.

Thanks for sharing. I'll try this approach next time.
Though, I think, this extra friction means that a lot of those cleanups just do not happen.
While I like git rebase I tend to slow down when I have 12 or more branches that I need to juggle.

Though, maybe, I just need to find a way to do it with GitHub =)

Yeah, I don't know of any way to use git to assign parts of the current changeset to separate commits, unless they are in separate files. I usually use interactive rebase and recreate the first sub-changeset. Works okay.

I know now to split changes in a single file into a separate commit.
One way to do it is to start an interactive rebase and stop before a commit you want to split.
Use git cherry-pick --no-commit or git show | git apply go get all changes from the commit you want to split.
Drop those you do not want to include in the first split commit (git restore --patch or other commands), and then create a commit out of the rest.
git rebase --continue will see that part of the change is already present and will only keep the missing part in the second commit.

But after I split changes into a separate commit, GitHub would not allow just this commit to be reviewed and approved.
Unless I put it on a separate branch to create a PR from it.
Which means, now I have the same commit in two branches, and I need to keep them in sync.

Gerrit, on the other hand, does not have a notion of PRs at all.
The whole workflow is focused on commits.
They are reviewed, approved and submitted.
So splitting changes into a separate commit, is all that you need.

If you do not care how changes are combined.
And you are OK with things being mixed up together, then GitHub is fine.
But if you really care about the review process and the produced commits, I think, Gerrit could be a better tool.

[mvines]

And you are OK with things being mixed up together, then GitHub is fine.

Not ok with things being mixed together. Sometimes it takes a bit to retrain your brain to work in the github model. But it's possible and it's generally fine. Yes, I fully agree Gerrit is better.