Use bytes from unrecorded_blocks rather from the block from DA

[MitchTurner]

Linked Issues/PRs

Closes #2244

Description

Essentially, we would be getting the compressed bytes from the committer, and we already have access to the true bytes in the unrecorded_blocks, so this PR changes the algo to use the values on those unrecorded_blocks

Uncovered some problems with the simulator in the mean time and created a followup issue for dealing with that.

Checklist

• New behavior is reflected in tests

Before requesting review

• I have reviewed the code myself

Followup Issue

#2264

[xgreenx]

Maybe we want to remove all blocks until height?

[rafal-ch]

I'm wondering, is it possible in a sunny-day scenario that we'll have some "lost", unrecorded blocks that will never be consumed by da_block_update()? I think yes, because update_da_record_data() gets an arbitrary set of blocks and we don't guarantee what are the heights of those blocks.

Maybe SkippedL2Block and SkippedDABlock errors protects us from this.

Anyway, leaving this comment for consideration as we might think about protecting unrecorded_blocks from growing indefinitely in case of some unexpected flow.

Edit:
I think that at some point we might need to take care about the size of unrecorded_blocks. It may happen that the user of the algorithm will populate the set by calling update_l2_block_data(), but will never call into update_da_record_data() to clear it. Maybe this is enforced on a higher level.
cc: @MitchTurner

[MitchTurner]

Maybe we want to remove all blocks until height?

That might be more performant (and kinda what we were doing before). We'd want to do that before we called da_block_update and pair each block_bytes with the corresponding RecordedBlock.

I honestly don't know the performance of remove for HashMap. get is O(1), but obviously doesn't mut so remove probably does re-scaling and other heap garbage. The most performant would be a VecDeque and split_off probably? The order is now an issue then. I was going to say we don't need to include the height (just the bytes) if we trust the order, but just in case we probably should and throw an error if it doesn't match the recorded_block it is paired with.

[MitchTurner]

Anyway, leaving this comment for consideration as we might think about protecting unrecorded_blocks from growing indefinitely in case of some unexpected flow.

Yeah. It's directly relevant to what we're talking about. We definitely make assumption about the order.

Talking to the rollup team guys, it sounds like they might not want to guarantee order in the long run, if that's the case then the HashMap approach might be the best. We could even get rid of SkippedL2Block I think... except we've lost info on the best cost_for_byte for L2 blocks that aren't in order.

[xgreenx]

I meant: Block committer submits blocks in the bundles. So bundle can have several blocks inside. When you receive the notification from the block commuter about DA submission, you can have unrecorded_blocks = vec![block_height-5, block_height-4, ... block_height]. Then instead of removing only one entry, you need to remove all entries up to block_height.

If that is something that we want, then BTreeMap is better=)

[MitchTurner]

We are currently removing all in the bundle. We are just iterating over the recorded blocks and removing one at a time:

    pub fn update_da_record_data(
        &mut self,
        blocks: &[RecordedBlock],
    ) -> Result<(), Error> {
        for block in blocks {
            self.da_block_update(block.height, block.block_cost)?;
        }
        self.recalculate_projected_cost();
        self.normalize_rewards_and_costs();
        Ok(())
    }

What I was saying is we could get them all first in some efficient way:

    pub fn update_da_record_data(
        &mut self,
        blocks: &[RecordedBlock],
    ) -> Result<(), Error> {
        let bytes = self.remove_bytes_for_recorded_blocks(&blocks)?;
        for (block, bytes) in blocks.iter().zip(bytes) {
            self.da_block_update(block.height, block.block_cost, bytes)?;
        }
        self.recalculate_projected_cost();
        self.normalize_rewards_and_costs();
        Ok(())
    }

Yeah maybe BTreeMap makes the most sense for the ordered case.

[xgreenx]

RecordedBlock data is linked to each other. It just has the latest block height

cc @rymnc Since he wrote down the interface that we agreed with Rollup team. Maybe I remember it incorrectly)

[MitchTurner]

Yes. I was part of those conversations and we've discussed what is required for the algo.

[rymnc]

This is what I have -

struct Bundle {
	sequence_number: u64,
	blocks_range: Range<BlockHeight>,
	// The DA block height of the last transaciton in the bundle.
	da_block_height: DaBlockHeight,
	// Total cost of all bundles for the whole history.
	total_cost: u256,
	// Total size of all bundles for the whole history.
	total_size: u256,
}

trait CommitterAPI {
	fn get_n_last_bundle(&self, number: u64) -> Result<Bundle>;

	// Range is based on `sequence_number`
	fn get_bundles_by_range(&self, range: Range<u64>) -> Result<Vec<Bundle>>;
}

[MitchTurner]

Right. Okay. So in that case we can just sum all the block bytes for that range and do a single cost calculation. Sounds like maybe that's what you were suggesting, @xgreenx . I was still under the impression that more processing would happen on the service side, but this actually works fine.

[rafal-ch]

I'm wondering, is it possible in a sunny-day scenario that we'll have some "lost", unrecorded blocks that will never be consumed by da_block_update()? I think yes, because update_da_record_data() gets an arbitrary set of blocks and we don't guarantee what are the heights of those blocks.

Maybe SkippedL2Block and SkippedDABlock errors protects us from this.

Anyway, leaving this comment for consideration as we might think about protecting unrecorded_blocks from growing indefinitely in case of some unexpected flow.

Edit:
I think that at some point we might need to take care about the size of unrecorded_blocks. It may happen that the user of the algorithm will populate the set by calling update_l2_block_data(), but will never call into update_da_record_data() to clear it. Maybe this is enforced on a higher level.
cc: @MitchTurner

[MitchTurner]

So, we can consider improving performance here still.

[MitchTurner]

Switched to BTreeMap and pop_first.

[rafal-ch]

LGTM 👍

Left two small nits.

[rafal-ch]

LGTM 👍

One possible issue for consideration: #2252 (comment)

This could be a non-issue or a real deal that we might consider handling as an follow-up issue.

[Dentosal]

Is this commented-out on purpose?

[MitchTurner]

Yes. I added the TODO with the related issue.

[Dentosal]

Do we want to keep this?

[MitchTurner]

This could be a non-issue or a real deal that we might consider handling as an follow-up issue.

@rafal-ch It's possible that we get into a situation that this is never shrunk. I'm not sure what the correct way to handle it is. I don't think it should be the worry of the gas price algorithm though. If the committer isn't responding for days or weeks, then the issue is going to need to be addressed anyway. We can always modify the algorithm if we recognize a problem in production.

[rafal-ch]

This could be a non-issue or a real deal that we might consider handling as an follow-up issue.

@rafal-ch It's possible that we get into a situation that this is never shrunk. I'm not sure what the correct way to handle it is.

It could be as simple as having a hardcoded or configured upper limit on unrecorded blocks and reject to accept more.

I don't think it should be the worry of the gas price algorithm though. If the committer isn't responding for days or weeks, then the issue is going to need to be addressed anyway.

True.
It could also happen due to a bug or an edge case in wiring up of the function calls, even if the committer is responding.

But yeah, since it's unlikely and of rather low impact we can move forward as is and observe how it behaves in production.

[Dentosal]

nit: this type seems complicated enough that a named type should make it more readable

[MitchTurner]

Done.

[Dentosal]

Suggested change

	.update_da_record_data(height..(height + 1), *cost)
	.update_da_record_data(height..=height, *cost)

[MitchTurner]

Unfortunately, RangeInclusive isn't the same type as Range, I went back and forth on whether to do a generic for the few traits I needed or just do Range and I figured this was less noise.

[MitchTurner]

Willing to reconsider if generics seems better.

[xgreenx]

Do we need changes from this file? Looks like @rymnc already migrated them and V1 will define own updater/service?

[MitchTurner]

Oh. Thanks. Good catch. I thought I deleted it in the merge but maybe I skipped it.