Fix set of indexes for processed txs in pending mbs

[SebastianMarian]

Description of the reasoning behind the pull request (what feature was missing / how the problem was manifesting itself / what was the motive behind the refactoring)

• Fixed set of indexes for processed txs in pending mini blocks, received in epoch start meta block, when a node starts in epoch before mini block partial execution feature is activated

Proposed Changes

Testing procedure

[iulianpascalau]

👍

[iulianpascalau]

so this composed condition will prevent the current edge case. Can we still have the scenario in which both fields are 0 (due to the unmarshaling of the empty slice) in which the if branch will still be executed?

[SebastianMarian]

No, because if reserved field will be nil, no matter of reason, the first line in the method returns TxCount-1

// GetIndexOfLastTxProcessed returns index of the last transaction processed in the miniBlock
func (m *MiniBlockHeader) GetIndexOfLastTxProcessed() int32 {
	miniBlockHeaderReserved, err := m.getMiniBlockHeaderReserved()
	if err != nil || miniBlockHeaderReserved == nil {
		return int32(m.TxCount) - 1
	}

[SebastianMarian]

And here we check only with GetIndexOfLastTxProcessed twice and not with both fields. GetIndexOfFirstTxProcessed does not matter when we try to determine if a block is partially executed. Only the index of the last tx executed will say if a miniblock is not yet fully processed.

[SebastianMarian]

A mini block referenced in a shard mini block header with indexOfFirstTxProcessed set to let's say: 0 or 5, could be partially executed or fully executed as well, in both cases, as all will depend by the value of the indexOfLastTxProcessed

[iulianpascalau]

there is a difference between reserved field nil or empty. Should we change the condition to:

if err != nil || len(miniBlockHeaderReserved) == 0 {

The marhaller will output an empty string for a struct with default values as can be seen in this test:

func Test2(t *testing.T){
    m := &marshal.GogoProtoMarshalizer{}

    b := &block.MiniBlockReserved{}

    buff, _ := m.Marshal(b)

    fmt.Println(buff)
    assert.NotNil(t, buff)
    assert.Equal(t, 0, len(buff))
}

[raduchis]

could be changed to len() to accommodate both the nil and empty

[SebastianMarian]

Actually miniBlockHeaderReserved could be only nil or NOT empty if you check the method m.getMiniBlockHeaderReserved() . There is no marshal but only unmarshal inside the method, so is not the case you underscored above. Anyways if we want this change, a new PR in elrond-go-core should be done. And also this change should be tested very well, as it will affect many parts of the code where the method is called.

// GetMiniBlockHeaderReserved returns the MiniBlockHeader Reserved field as a MiniBlockHeaderReserved
func (m *MiniBlockHeader) getMiniBlockHeaderReserved() (*MiniBlockHeaderReserved, error) {
	if len(m.Reserved) > 0 {
		mbhr := &MiniBlockHeaderReserved{}
		err := mbhr.Unmarshal(m.Reserved)
		if err != nil {
			return nil, err
		}

		return mbhr, nil
	}
	return nil, nil
}

[iulianpascalau]

return nil, nil. That is another red flag

[SebastianMarian]

This is for backwards compatibility as reserved was not used before scheduled miniblocks and here we can not pass the epoch notifier

[iulianpascalau]

still not agree. It can easily raise panics as we got used to use the code

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (rc/2022-july@df68515). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             rc/2022-july    #4392   +/-   ##
===============================================
  Coverage                ?   73.83%           
===============================================
  Files                   ?      675           
  Lines                   ?    86847           
  Branches                ?        0           
===============================================
  Hits                    ?    64120           
  Misses                  ?    17933           
  Partials                ?     4794           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[raduchis]

could be changed to len() to accommodate both the nil and empty

[gabi-vuls]

System test passed.