eof: new contract creation #15512

rodiazet · 2024-10-14T10:03:00Z

~~Depends on #15456.~~ Merged
~~Depends on #15521~~ Dropped
~~Depends on: #15529~~ Merged
Depends on #15536
Depends on #15535

github-actions · 2024-10-14T10:03:13Z

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

cameel

I only skimmed through the PR for now. Here's some initial feedback.

My main question would be whether it wouldn't make more sense to deal with loadimmutable/setimmutable in a separate PR first. It's related dataloadn and looks like creation depends on immutables to some extent.

cameel · 2024-10-14T21:57:08Z

libevmasm/Assembly.cpp

+			case EofCreate:
+			{
+				ret.bytecode.push_back(static_cast<uint8_t>(Instruction::EOFCREATE));
+				ret.bytecode.push_back(static_cast<uint8_t>(item.data()));
+				break;
+			}
+			case ReturnContract:
+			{
+				ret.bytecode.push_back(static_cast<uint8_t>(Instruction::RETURNCONTRACT));
+				ret.bytecode.push_back(static_cast<uint8_t>(item.data()));
+				break;
+			}


Also probably worth considering whether we should maybe create a general assembly item for operations with immediates like I mentioned in #15456 (comment). Though I guess we'd need more than one to accommodate different numbers of immediates and their sizes?

I'm not sure about this and it's also something we can refactor later, but I just wanted to put the general idea for discussion.

cameel · 2024-10-14T21:58:50Z

libevmasm/AssemblyItem.h

+	/// Loads 32 bytes from static auxiliary data of EOF data section. The offset does *not* have to be always from the beginning
+	/// of the data EOF section. More details here: https://github.com/ipsilon/eof/blob/main/spec/eof.md#data-section-lifecycle
+	AuxDataLoadN,
+	EofCreate, /// Creates new contract using subcointainer as initcode


Suggested change

EofCreate, /// Creates new contract using subcointainer as initcode

EOFCreate, /// Creates new contract using subcontainer as initcode

cameel · 2024-10-14T22:06:04Z

libsolidity/codegen/ir/IRGenerator.cpp

+		t("library_address", IRNames::libraryAddressImmutable());
+	else
+		// TODO: We assume that libraries do not have immutable variables. The address is stored in 0 offset of
+		//  auxiliary data section. Is it correct?


I think this is a correct assumption. Libraries cannot have state variables and this immutable for address is the only one I'm aware of.

I think that even if it's not correct, I we'll easily find out soon enough because we'll have to deal with all the loadimmutable calls in the generator anyway so no need to worry too much.

BTW, I think it might be best to deal with the immutable-related builtins first (loadimmutable, setimmutable). I see several options:

remove them from the dialect when EOF is enabled.

implement them on EOF using auxdataload().

implement them on EOF, but make auxdataload()`'s 0 offset start after them.

Going with (2) or (3) would actually allow you not to care about it here at all. You'd just keep using the same immutable. You'd instead have to adjust the mechanism for allocating immutables to put them in aux data section.

I'm not sure whether (2) or (3) is preferable though.

Ok. Option 1 is implemented here but I will investigate other options and we can decide which is the best.

cameel · 2024-10-18T21:06:50Z

libevmasm/Assembly.cpp

+				solAssert(item.data() <= std::numeric_limits<uint8_t>::max(),
+					"Invalid EofCreate/ReturnContract index value.");


Shouldn't this be checked against the number of subcontainers instead?

Also:

Suggested change

solAssert(item.data() <= std::numeric_limits<uint8_t>::max(),

"Invalid EofCreate/ReturnContract index value.");

solAssert(

item.data() <= std::numeric_limits<uint8_t>::max(),

"Invalid subcontainer index."

);

cameel · 2024-10-18T21:26:12Z

libevmasm/SemanticInformation.cpp

EOFCREATE should be added to readWriteOperations(), isDeterministic() and memory() too.

RETURNCONTRACT should be added to memory(). Maybe also otherState() since it results in the code being stored? Not sure about invalidInViewFunctions() - what happens if someone tries to use it in a STATICCALL?

RETURNCONTACT can be called only in context of EOFCREATE. And EOF create context should revert or call RETURNCONTRACT

https://eips.ethereum.org/EIPS/eip-7620#returncontract

cameel · 2024-10-18T21:40:13Z

libevmasm/SemanticInformation.cpp

+	case Instruction::RETURNCONTRACT:
 		return true;


This is not enough. This switch only handles the Operation item. You need to add the new item to the condition above as well.

Same with terminatesControlFlow() below.

cameel · 2024-10-18T22:05:39Z

test/libyul/objectCompiler/eof/auxdata_load_store.yul

+            returncontract("b", 0, 32)
+            return(0, 32)


Suggested change

returncontract("b", 0, 32)

return(0, 32)

returncontract("b", 0, 32)

BTW. This reminds me that returncontract cannot continue. Does yul verify that there are not unreachable instructions?

Not sure actually. Probably not. We have a warning at Solidity level but Yul analysis is very simple so I wouldn't be surprised if we don't detect it. I did a quick check by compiling a contract and I did not get such a warning.

Unreachable Yul code is eventually removed by the optimizer anyway.

cameel · 2024-10-21T17:36:14Z

libsolidity/codegen/ir/IRGenerator.cpp

+	else
+	{
+		auto const& immutableVariables = ContractType(_contract).immutableVariables();
+		auto const varIt = std::find_if(immutableVariables.begin(), immutableVariables.end(), [](VariableDeclaration const* _varDecl) { return _varDecl->name() == IRNames::libraryAddressImmutable(); });
+		solAssert(varIt != immutableVariables.end(), "");
+		t("library_address_offset", std::to_string(m_context.immutableMemoryOffset(*(*varIt))));
+	}


This should be a helper on IRGenerationContext - basically a new overload of immutableMemoryOffset() that accepts the name and searches m_immutableVariables.

It would also be safer - searching through ContractType::immutableVariables() will probably give you the same immutables, but I'm not 100% sure there isn't some corner case where they could differ.

This idea was dropped and now we assume that library address is in 0 offset of aux data. The address is not stored in reserverMemory. I though it is.

cameel · 2024-10-21T17:38:54Z

libsolidity/codegen/ir/IRGenerator.cpp

+			if (!eof)
+				t("id", std::to_string(_varDecl.id()));
+			else
+				t("immutableDataSectionOffset", std::to_string(m_context.immutableMemoryOffset(_varDecl)));


This is not an offset relative to the start of data section but rather aux data. I'd rename it to avoid misunderstandings:

Suggested change

t("immutableDataSectionOffset", std::to_string(m_context.immutableMemoryOffset(_varDecl)));

t("immutableAuxDataOffset", std::to_string(m_context.immutableMemoryOffset(_varDecl)));

Also, note that immutableMemoryOffset() gives you an absolute memory address. I'm not sure it's suitable for use as data section offset. It needs to be made relative and also, I'm not sure these offsets are contiguous.

Ohh shit. You are right. It' relative. I will change accordingly.

cameel · 2024-10-21T17:58:41Z

libsolidity/codegen/ir/IRGenerator.cpp

+				immutables.emplace_back(std::map<std::string, std::string>{
+					{"immutablesMemoryStart"s, std::to_string(CompilerUtils::generalPurposeMemoryStart)},
+					{"immutablesOffset"s, std::to_string(m_context.immutableMemoryOffset(*immutable))},
+					{"value"s, "address()"}


This looks like it was copied from the library case. It should be the value of the immutable instead. The non-EOF branch has

"mload(" + std::to_string(m_context.immutableMemoryOffset(*immutable)) + ")"

Legacy for some reason makes this the same way. I assume for library the address it not stored in reservedMemory.

cameel · 2024-10-21T18:08:16Z

libsolidity/codegen/ir/IRGenerator.cpp

+					{"immutablesMemoryStart"s, std::to_string(CompilerUtils::generalPurposeMemoryStart)},
+					{"immutablesOffset"s, std::to_string(m_context.immutableMemoryOffset(*immutable))},


immutablesMemoryStart will stay the same for each element, won't it? It can be a normal template parameter, outside of immutables.

Also, immutableMemoryOffset() already gives you an absolute memory address, so you don't even need to add immutablesMemoryStart:

solidity/libsolidity/codegen/ir/IRGenerationContext.cpp

Line 96 in abc46f3

m_immutableVariables[&_variable] = CompilerUtils::generalPurposeMemoryStart + *m_reservedMemory;

It's dropped now. The idea was to have an interface with auxdatastore which accepts a memory pointer where the data which is going to be appended to the container starts. Then a relative offset to immutable and the value. In such a solution you would be able to store values in any memory offset and pass it to returncontract.

cameel · 2024-10-21T18:12:29Z

libsolidity/codegen/ir/IRGenerator.cpp

+			<#immutables>
+				auxdatastore(<immutablesMemoryStart>, <immutablesMemoryOffset>, <value>)
+			</immutables>
+			returncontract("<object>", <immutablesOffset>, <immutablesSize>)


immutablesMemoryOffset seems undefined. On the other hand immutablesOffset is defined for each item, but you're using it outside of the loop.

I'm not entirely sure which idea this is implementing (there were a few discussed on the EOF channel) but it does not look correct yet. The auxdatastore() clearly copies the values of the immutables, but to where? You're not allocating any memory for them.

It would actually be best not to have to copy them in the first place. They're already stored in reserved memory so just pass that memory to returncontract. The only complication might be whether (1) we can guarantee that only immutables are in this reserved memory and (2) that they're in the right order. I'm not sure about either without diving deeper into the code.

There are only immutables in the memory. The order does matter only on loading them to stack with auxdataloadn. The offset is taken from IRGeneratorContext

cameel · 2024-10-21T18:27:25Z

libsolidity/experimental/codegen/IRGenerator.cpp

+				<?eof>
+					returncontract("<DeployedObject>", 0, 0)
+				<!eof>


We don't have datasize()/dataoffset() on EOF yet, so I guess this case should be guarded with solUnimplemented().

We should also not have datasize(), dataoffset(), datacopy(), setimmutable(), loadimmutable() defined in the Dialect on EOF since they aren't really supported there (at least not in the current form).

I don't fully get it. For EOF in this case we don't don't use this data related function here. Or maybe I don't understand what is experimental codegen about. :)

Moreover on Dialect level these functions are disabled in EOF

Also note that this is in libsolidity/experimental (and thus doesn't really matter) - we wanted to remove the entire thing anyways, didn't we? (Since we moved to an external prototype for it and all)

cameel · 2024-10-21T18:36:42Z

libyul/AsmAnalysis.cpp

+					// FIXME: m_dataNames contains root object name which means we can call EOF creation function with
+					// root container which is wrong. Is should be possible to call these functions with direct
+					// subcontainers only.


I guess we need to pass in the name of the current object to the analyzer.

As for subcontainers, you can check if the name contains dots (unless it's prefixed with the current object name). This should be a static helper in Object.

Naming subcontainers with dots is just a convention and it's not required by yul right? If so we cannot rely in this assumption. I will pass current object name to the analyzer.

cameel · 2024-10-21T18:37:52Z

libyul/AsmAnalysis.cpp

+					// FIXME: m_dataNames contains root object name which means we can call EOF creation function with
+					// root container which is wrong. Is should be possible to call these functions with direct
+					// subcontainers only.


I guess we need to pass in the name of the current object to the analyzer.

As for subcontainers, you can check if the name contains dots (unless it's prefixed with the current object name). This should be a static helper in Object.

cameel · 2024-10-21T18:58:55Z

libyul/backends/evm/EVMDialect.cpp

+						yulAssert(_call.arguments.size() == 3, "");
+						Literal const* literal = std::get_if<Literal>(&_call.arguments.front());
+						yulAssert(literal, "");
+						auto const it = context.subIDs.find(formatLiteral(*literal));
+						yulAssert(it != context.subIDs.end(), "");
+						yulAssert((*it).second <= std::numeric_limits<solidity::yul::AbstractAssembly::ContainerID>::max(), "");
+						_assembly.appendReturnContract(static_cast<solidity::yul::AbstractAssembly::ContainerID>((*it).second));


This could be simplified a bit:

The message in asserts is optional.

We're in solidity::yul namespace here.

I'd avoid micro-optimizing map lookups or variant casts, at least not in non-performance critical code. They usually makes things hard to read and is not worth it IMO.

We also have valueOrDefault(), valueOrNull(), which should actually make it just as efficient as with iterators.

Suggested change

yulAssert(_call.arguments.size() == 3, "");

Literal const* literal = std::get_if<Literal>(&_call.arguments.front());

yulAssert(literal, "");

auto const it = context.subIDs.find(formatLiteral(*literal));

yulAssert(it != context.subIDs.end(), "");

yulAssert((*it).second <= std::numeric_limits<solidity::yul::AbstractAssembly::ContainerID>::max(), "");

_assembly.appendReturnContract(static_cast<solidity::yul::AbstractAssembly::ContainerID>((*it).second));

yulAssert(_call.arguments.size() == 3);

yulAssert(std::holds_alternative<Literal>(_call.arguments.front()));

std::string subObjectName = formatLiteral(std::get<Literal>(_call.arguments.front()));

AbstractAssembly::SubID* containerID = util::valueOrNull( context.subIDs, subObjectName);

yulAssert(containerID && *containerID <= std::numeric_limits<AbstractAssembly::ContainerID>::max());

_assembly.appendReturnContract(static_cast<AbstractAssembly::ContainerID>(*containerID);

cameel · 2024-10-21T19:10:11Z

libyul/backends/evm/EVMDialect.cpp

+				// TODO: Can it be done better? For instructions is being done by `createEVMFunction`
+				createdFunctionIt->second.controlFlowSideEffects.canContinue = false;
+				createdFunctionIt->second.controlFlowSideEffects.canTerminate = true;
+				createdFunctionIt->second.controlFlowSideEffects.canRevert = false;


I guess we could add ControlFlowSideEffects as a parameter in createFunction(). And also extract the part of createEVMFunction() that fills it out based on SemanticInformation of a specific instruction into a helper.

In this PR?

Ok. I see the last comment now. Will push PR soon

cameel · 2024-10-21T19:18:34Z

If you want, some of the things mentioned above ther could potentially be put in a separate PR(s) to make this one smaller:

Disabling datasize(), dataoffset(), datacopy(), setimmutable(), loadimmutable() on EOF (eof: new contract creation #15512 (comment)).
Adding ControlFlowSideEffects to createFunction() (eof: new contract creation #15512 (comment))
Making the name of the current object available in AsmAnalyzer (eof: new contract creation #15512 (comment)).

github-actions bot added the external contribution ⭐ label Oct 14, 2024

rodiazet force-pushed the eof-contract-creation branch 4 times, most recently from f8a9ae7 to 947c5dd Compare October 14, 2024 13:36

cameel added EOF has dependencies The PR depends on other PRs that must be merged first labels Oct 14, 2024

cameel changed the title ~~Eof contract creation~~ EOF contract creation Oct 14, 2024

cameel reviewed Oct 14, 2024

View reviewed changes

rodiazet force-pushed the eof-contract-creation branch 2 times, most recently from 6567616 to 82930c3 Compare October 15, 2024 13:06

cameel removed the has dependencies The PR depends on other PRs that must be merged first label Oct 16, 2024

rodiazet force-pushed the eof-contract-creation branch 2 times, most recently from feeb58b to 75122e0 Compare October 18, 2024 13:23

This comment was marked as resolved.

Sign in to view

rodiazet force-pushed the eof-contract-creation branch 2 times, most recently from 7bde2af to e1840a3 Compare October 18, 2024 13:29

rodiazet changed the title ~~EOF contract creation~~ eof: new contract creation Oct 18, 2024

rodiazet force-pushed the eof-contract-creation branch 3 times, most recently from a200d15 to 107b86e Compare October 18, 2024 14:11

cameel added the has dependencies The PR depends on other PRs that must be merged first label Oct 18, 2024

rodiazet force-pushed the eof-contract-creation branch from 107b86e to 2bb05a2 Compare October 18, 2024 18:02

cameel reviewed Oct 18, 2024

View reviewed changes

rodiazet force-pushed the eof-contract-creation branch 5 times, most recently from def54bb to 46af949 Compare October 21, 2024 11:05

rodiazet marked this pull request as ready for review October 21, 2024 11:06

rodiazet force-pushed the eof-contract-creation branch from 46af949 to a764163 Compare October 21, 2024 12:54

cameel mentioned this pull request Oct 21, 2024

eof: auxdatastore as a tool to store immutables in data section of EOF #15521

Closed

cameel requested changes Oct 21, 2024

View reviewed changes

rodiazet force-pushed the eof-contract-creation branch 2 times, most recently from 2f36738 to c9dd5ec Compare October 22, 2024 12:45

rodiazet added 2 commits October 22, 2024 15:46

Add ControlFlowSideEffects parameter to createFunction in EVMDialect

edd6b02

Pass object name to AsmAnalyzer

58bfddb

rodiazet force-pushed the eof-contract-creation branch from c9dd5ec to e4550e5 Compare October 22, 2024 14:02

eof: Support EOF contract creation.

4da1267

rodiazet force-pushed the eof-contract-creation branch from e4550e5 to 4da1267 Compare October 22, 2024 14:11

	EofCreate, /// Creates new contract using subcointainer as initcode
	EOFCreate, /// Creates new contract using subcontainer as initcode

		solAssert(item.data() <= std::numeric_limits<uint8_t>::max(),
		"Invalid EofCreate/ReturnContract index value.");

	returncontract("b", 0, 32)
	return(0, 32)
	returncontract("b", 0, 32)

	t("immutableDataSectionOffset", std::to_string(m_context.immutableMemoryOffset(_varDecl)));
	t("immutableAuxDataOffset", std::to_string(m_context.immutableMemoryOffset(_varDecl)));

		{"immutablesMemoryStart"s, std::to_string(CompilerUtils::generalPurposeMemoryStart)},
		{"immutablesOffset"s, std::to_string(m_context.immutableMemoryOffset(*immutable))},

eof: new contract creation #15512

Are you sure you want to change the base?

eof: new contract creation #15512

Conversation

rodiazet commented Oct 14, 2024 • edited Loading

github-actions bot commented Oct 14, 2024

cameel left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Oct 21, 2024

rodiazet commented Oct 14, 2024 •

edited

Loading

cameel left a comment •

edited

Loading

cameel Oct 14, 2024 •

edited

Loading

rodiazet Oct 22, 2024 •

edited

Loading

cameel Oct 21, 2024 •

edited

Loading