go-ethereum icon indicating copy to clipboard operation
go-ethereum copied to clipboard

accounts/abi/bind/backends: fix AdjustTime to respect Fork

Open lightclient opened this issue 3 years ago • 2 comments

closes #25220

lightclient avatar Jul 01 '22 17:07 lightclient

Rollback() also ignores Fork(), but I think that is okay because it allows me to rollback the fork command if I need to

MariusVanDerWijden avatar Jul 04 '22 11:07 MariusVanDerWijden

Interesting point, Rollback() ignoring Fork() does mean that you can undo the last Fork call by rolling back the entire side-chain (only keeping the canonical chain). However, it doesn't match the comment on Rollback:

Rollback aborts all pending transactions, reverting to the last committed state.

Something like the following would, for example, revert the call to Fork, too, even though the last committed state is in the side-chain:

sb.Commit()
sb.Commit()
sb.Commit()
sb.Fork(...) // Back to the first Commit()
sb.Commit()
sb.SendTransaction(...)
sb.Rollback()

According to the comment on Rollback this should only remove the SendTransaction call, the current implementation goes back to the canonical chain and thus removes all calls after Fork unless that is the canonical chain (in this case Fork, the last Commit and SendTransaction). Additionally, the current implementation rolls back all Forks (since you could call Fork with a block that is already in a side-chain. Wouldn't it be more intuitive if Rollback either undoes the last Fork or doesn't bother with Fork at all (like the comment suggests). The latter would probably be easier to implement, since it could just use b.pendingBlock.ParentHash.

Another thing to consider is, that (with the current implementation) you'd have to use Fork (on the latest block of the side-chain) to undo pending transactions, because otherwise you'd roll back to the canonical chain. To undo a call to Fork (like rollback currently does) you could always call sb.Fork(ctx, sb.Blockchain().CurrentHeader().Hash()), which could also be a RollbackToCanonicalChain function.

(Just my 2ct, I'd be fine with both versions, though I'd suggest to change the rollback comment if it rolls back any Fork calls to go to the canonical chain)

DragonDev1906 avatar Jul 04 '22 14:07 DragonDev1906

~~@lightclient please rebase~~ nevermind, I did it

holiman avatar Sep 26 '22 13:09 holiman