Remove go-interop feature from the AMT

[Stebalien]

I believe this feature exists solely to ensure that gas usage matched go. Now that we compile everything to wasm, we should be able to remove it.

ref-fvm/ipld/amt/Cargo.toml

Line 21 in 9e2b2af

go-interop = []

ref-fvm/ipld/amt/src/amt.rs

Lines 487 to 547 in 9e2b2af

	pub fn for_each_while_mut<F>(&mut self, mut f: F) -> Result<(), Error>
	where
	// TODO remove clone bound when go-interop doesn't require it.
	// (If needed without, this bound can be removed by duplicating function signatures)
	V: Clone,
	F: FnMut(u64, &mut ValueMut<'_, V>) -> anyhow::Result<bool>,
	{
	#[cfg(not(feature = "go-interop"))]
	{
	let (_, did_mutate) = self.root.node.for_each_while_mut(
	&self.block_store,
	self.height(),
	self.bit_width(),
	0,
	&mut f,
	)?;
	if did_mutate {
	self.flushed_cid = None;
	}
	Ok(())
	}
	// TODO remove requirement for this when/if changed in go-implementation
	// This is not 100% compatible, because the blockstore reads/writes are not in the same
	// order. If this is to be achieved, the for_each iteration would have to pause when
	// a mutation occurs, set, then continue where it left off. This is a much more extensive
	// change, and since it should not be feasibly triggered, it's left as this for now.
	#[cfg(feature = "go-interop")]
	{
	let mut mutated = Vec::new();
	self.root.node.for_each_while_mut(
	&self.block_store,
	self.height(),
	self.bit_width(),
	0,
	&mut |idx, value| {
	let keep_going = f(idx, value)?;
	if value.value_changed() {
	// ! this is not ideal to clone and mark unchanged here, it is only done
	// because the go-implementation mutates the Amt as they iterate through it,
	// which we cannot do because it is memory unsafe (and I'm not certain we
	// don't have side effects from doing this unsafely)
	value.mark_unchanged();
	mutated.push((idx, value.clone()));
	}
	Ok(keep_going)
	},
	)?;
	for (i, v) in mutated {
	self.set(i, v)?;
	}
	Ok(())
	}
	}

But I lack the context to say for certain.

[LesnyRumcajs]

Currently, Forest doesn't use the go-interop feature for fvm_ipld_amt. We might need it in the future when dealing more with the pre-FVM network versions, though I'm not sure that a feature flag is the way to go in such a case.

As it stands, I'm okay with removing it and to look for an alternate solution when a need arises.

@lemmih are you okay with this?

[lemmih]

Yes, I think we should remove it.

[Stebalien]

Note: this likely requires modifying the builtin actors. Anyone from your team want to take this?

[LesnyRumcajs]

I believe this would be me! To make sure the workflow between builtin-actors and ref-fvm is understood by me:

1. Do the change in ref-fvm.
2. Do the change in builtin-actors with this to ensure all is still good.
3. PR with getting rid of go-interop in this repo.
4. After merging, nag someone for a release.
5. PR in builtin-actors with the updated fvm_ipld_amt version.

Is this correct? Am I missing anything or any other repo the change should be reflected in?

[Stebalien]

Yes. Although you may be able to just change the builtin actors first to avoid that dance (as far as I can tell, the change here just involves removing the feature, so you should be able to disable that feature in the builtin actors first).