Issue with panic in SetCell

[Wondertan]

Context

Today, we panic when we attempt to set a non-nil cell. However, there is a case where this becomes problematic. The logic in celestia-node runs the Repair method several times, which partially repairs the EDS and only some rows and cols on each run. The networking logic does not know which ones were repaired, and may download a share that was already repaired, subsequently setting into the square and panic.

Workaround

The workaround is to call GetCell before SetCell to check if the cell is already there. However, the function does unnecessary copying inside.

Possible solutions

• HasCell that only report true or false
  • This could work, but it's not clear why we need to panic and perform the check twice: (1) HasCell by user and in SetCell in the actual logic

if !square.HasCell(x, y) {
  square.SetCell(x, y, cell)
}

• Don't panic in SetCell and return bool instead, which reports whether setting a cell succeeded

[adlerjohn]

and return bool instead

Should it return an error instead?

[Wondertan]

@adlerjohn, It could. Don't think it really matters. From a high level, I would say it's fine to make it an acceptable case as we inherently have two commits to every cell. The networking layer, in the worst case, uses both to get and set the share, and it seems normal for rsmt2d to be aware of this by not erroring on such a case. Error and panic are something incorrect, while this case does seem to be expected.

Anyway, I don't feel strongly about this and am happy to go with any solution you feel is the best.