libdriver #24

mpolitzer · 2023-10-11T14:15:25Z

THIS IS WORK IN PROGRESS

Work being done in the context of: cartesi/machine-emulator#137.

implement a library for the device drivers.
implement a library for EVM calldata and outputs_root_hash calculations.
renamed to libcmt.
implement bundling and the installation process.
port existing tools to this library (HTTP server/client is an exception and will be done in: Support Output Unification rollups-node#103 context).

linux/libdriver/examples/rollup.c

linux/libdriver/base/rollup-driver-api.h

diegonehab

See comments.

linux/libdriver/base/yield-driver-api.h

linux/libdriver/base/keccak.c

linux/libdriver/ioctl/yield.c

linux/libdriver/base/abi.h

linux/libdriver/base/abi.c

edubart · 2023-10-26T21:08:01Z

linux/libdriver/base/buf.c

+}
+void evm_buf_xxd(void *p, void *q, int l)
+{
+	assert(p);


Better convert these asserts into returning error codes.

edubart · 2023-10-26T21:48:47Z

linux/libdriver/ioctl/rollup.c

+{
+	munmap(me->tx->data, me->tx->length);
+	munmap(me->rx->data, me->rx->length);
+	close(me->fd);


It's better to reset all me fields when uninitializing, in case the user tries to call rollup driver functions again the failure will be more deterministic. A detail is that fd must not be set to 0, but to -1.

edubart · 2023-10-26T21:51:36Z

linux/libdriver/ioctl/rollup.c

+
+void *rollup_driver_get_tx(struct rollup_driver *me, size_t *max)
+{
+	*max = me->tx->length;


Better check if max is not NULL, and only set it in this case. Same for the function below.

edubart · 2023-11-03T16:14:27Z

linux/libdriver/base/keccak.h

@@ -0,0 +1,131 @@
+/** @file
+ * @defgroup libevm_keccak keccak
+ * Keccak 256 digest


It should be noted that this implementation does not follow the FIPS-202 based standard (aka finalized NIST SHA-3), it follows the Ethereum Keccak-256.

diegonehab

See comments.

diegonehab · 2023-11-10T15:44:18Z

linux/libdriver/base/merkle.h

+ *
+ * @param [in]  me   initialized state
+ * @param [out] root root hash of the merkle tree */
+void evm_merkle_get(evm_merkle_t *me, uint8_t root[EVM_KECCAK_MDLEN]);


Can we follow the same names as the ones in back-merkle-tree.cpp from the emulator repo? It's the same functionality, after all. So evm_merkle_get_root_hash, evm_merkle_push_back, evm_merkle_push_back_from_data. I don't think you need evm_merkle_pad_back, but we might at some point.

diegonehab · 2023-11-10T15:46:00Z

linux/libdriver/base/abi.h

+ * @note @p me must outlive @p res.
+ * Create a duplicate otherwise */
+int
+evm_abi_res_bytes_d(evm_buf_t *me, evm_buf_t *of, size_t n, evm_buf_t *out, const void *start);


You really don't like writing. :) I think Eduardo also doesn't like writing long names. But I would use reserve because most people will think res means result.

diegonehab · 2023-11-10T15:46:55Z

linux/libdriver/base/abi.h

+ * @param [in]  data integer value to decode into @p out
+ * @param [in]  n    size of @p data in bytes
+ * @param [out] out  decoded output */
+int evm_abi_dec_uint(const uint8_t data[EVM_WORD_LEN], size_t n, uint8_t out[n]);


Same for encode, decode. In fact, dec almost always means decrement, decrease.

diegonehab · 2023-11-10T15:47:44Z

linux/libdriver/base/buf.h

+/** A slice of contiguous memory from @b p to @b q.
+ * Size can be taken with: `q - p`. */
+typedef struct {
+	uint8_t *p; /**< start of memory region */


Here as well. Why call something p and q if you can write begin and end. :)

And is this inclusive or not? If you use begin, end, most C++ programmers will understand that begin == end means the buffer is empty.

Please don't use end because it is a reserved keyword for example in Lua, and what if someone wants to bind this to Lua, although this file is to low level to be exposed in Lua. You could prefix with something or could use start and finish instead. Just be careful with names that are reserved keywords in other languages.

If it was in Lua, it would be a field in a table, where end is not reserved. It’s a bit more awkward to use, but it would still work. I have no idea what the reserved words are in Go or Rust. Anyway, start and finish is better than p and q. ;)

Ok, changing it to begin, end pair.

diegonehab · 2023-11-10T15:50:14Z

linux/libdriver/base/buf.h

+ *
+ * @note @p data memory must outlive @p me.
+ * user must copy the contents otherwise */
+void evm_buf_init(evm_buf_t *me, size_t n, void *data);


We have been using len or length for the size of things. Now n. I suggest you always use length.

I usually like size because it matches the same number of characters of data :), but I am fine with any of the names.

That is some next level OCD right there. 😂 Anyway, I just think we should stick to one word for the concept.

diegonehab · 2023-11-10T16:03:56Z

linux/libdriver/base/rollup-driver-api.h

+ * @return
+ * - 0 on success
+ * - negative value on error. errno values from `ioctl` */
+int rollup_driver_revert(struct rollup_driver *me);


So, the low-level driver has this more general API, in which you could potentially call revert down some call chain, rather than having to pass some flag up the chain until the call to finish. But the higher level API will still be like the old finish. Is that the idea?

correct, it also maps closer to the new driver implementation.

diegonehab · 2023-11-10T16:04:35Z

linux/libdriver/base/rollup-driver-api.h

+ * @return
+ * - pointer to the buffer.
+ * @note memory is valid until @ref rollup_driver_fini is called. */
+void *rollup_driver_get_tx(struct rollup_driver *me, size_t *max);


I'd use length instead of max, for consistency.

diegonehab · 2023-11-10T16:08:01Z

linux/libdriver/base/rollup-driver-api.h

+ *
+ * @param [in] me A sucessfuly initialized state by @ref rollup_driver_init
+ * @param [in] accept @p true if this input was processed successfuly
+ * @param [out] n size in bytes of the current input.


I'd use length again. And would say that is the length in bytes of the newly received input. "Current" is kind of ambiguous in a function that asks for the "next" input.

diegonehab · 2023-11-10T16:09:42Z

linux/libdriver/base/rollup.h

+ * - @ref evm_rollup_init */
+typedef struct {
+	struct rollup_driver rollup_driver[1];
+	evm_buf_t tx[1],


Do you declare tx[1] as some aesthetic trick so you can write rollup->tx->length instead of rollup->tx.length? I have never seen this but I like it. :)

Yes, basically syntactic sugar to pass F(x) instead of F(&x) everywhere.

diegonehab · 2023-11-10T16:11:01Z

linux/libdriver/base/rollup.h

+
+typedef struct {
+	uint8_t sender[EVM_ADDRESS_LEN];
+	uint64_t blockNumber;


I don't think we camelCase in any of our APIs. Can se use the same names we used in input_metadata, to the extent possible?

Got it from here: cartesi/rollups-contracts#84, but I can change to the one we used on the driver. Will cause less changes in other the other tools as well.

I’d stick with the convention in the C structure for input_metadata we had before.

edubart · 2023-11-16T13:29:52Z

linux/libcmt/src/merkle.c

+	if (me->n == UINT64_MAX)
+		return -ENOBUFS;
+
+	unsigned n = ((uint64_t)ffsll(++me->n)-1u);


src/merkle.c: In function ‘cmt_merkle_push_back’: src/merkle.c:37:33: warning: implicit declaration of function ‘ffsll’ [-Wimplicit-function-declaration] 37 | unsigned n = ((uint64_t)ffsll(++me->n)-1u);

edubart · 2023-11-16T13:58:38Z

linux/libcmt/Makefile

+	cp -f doc/html $(DESTDIR)$(PREFIX)/share/libcmt/html
+
+clean:
+	rm -f $(mock_BINS) $(ioctl_BINS) $(tests_BINS) $(tools_BINS) $(OBJ) $(OBJ:%.o=%.d)


It's not cleaning libcmt_mock.a yet.

edubart · 2023-11-16T13:59:06Z

linux/libcmt/Makefile

+	cp -f include/libcmt/*.h $(TARGET_DESTDIR)$(TARGET_PREFIX)/include/libcmt/
+	cp -f include/libcmt/ioctl/*.h $(TARGET_DESTDIR)$(TARGET_PREFIX)/include/libcmt/
+	mkdir -p $(TARGET_DESTDIR)$(TARGET_PREFIX)/lib/pkgconfig
+	sed 's|ARG_PREFIX|$(TARGET_PREFIX)|g' tools/templates/libcmt.pc \


The .pc files in tools/templates/ are missing in the repo.

edubart · 2023-11-16T14:05:41Z

linux/libcmt/Makefile

+doc.build: examples.build doc/theme
+	doxygen doc/Doxyfile
+
+ioctl.install: libcmt.a


For binding in other programming languages that cannot link to C static libraries, we also want to install a shared library, and we have to make sure all public C APIs are exported to the .so file, probably need to add CMT_API define to every public function.

My understanding is that those languages need some extra glue code, the way Lua does it comes to mind.
Wouldn't that be layer responsible for creating the .so as well?

Many can just dlopen and FFI. So I think it’s good to have a .so.

edubart · 2023-11-16T14:20:31Z

linux/libcmt/include/libcmt/rollup.h

+                             ,cmt_rollup_inspect_t *inspect);
+
+/** 
+ *


Documentation is missing, making the function not show up in Doxygen.

edubart · 2023-11-16T14:50:51Z

linux/libcmt/include/libcmt/ioctl/rollup-driver.h

+		uint64_t length;
+	} tx[1], rx[1];
+};
+#include <libcmt/rollup-driver-api.h>


Use relative includes, so we can allow people to copy these include headers in non standard directories.

Unless these files are not going to be installed in the public API, then maybe the should be moved to src directory.

edubart · 2023-11-16T15:00:43Z

linux/libcmt/examples/rollup-driver.c

+		switch (type) {
+		case CMT_ROLLUP_ADVANCE_STATE:
+			memcpy(tx, rx, n); /* echo */
+			cmt_rollup_driver_write_output(R, n);


All official examples deserve proper error handling for all functions, to no influence people not caring about errors. I noticed this example is in the documentation, so changes here should reflect the documentation.

It is possible to generate warnings about unused error codes by adding something like

#define CMT_API __attribute__((__warn_unused_result__)) CMT_API int cmt_rollup_driver_write_output(struct cmt_rollup_driver *me, uint64_t n);

just temporality, so you can make sure all the code base is considering the errors.

edubart · 2023-11-16T15:09:57Z

linux/libcmt/src/ioctl/rollup-driver.c

+
+void *cmt_rollup_driver_get_tx(struct cmt_rollup_driver *me, size_t *max)
+{
+	*max = me->tx->length;


We should not crash if max is NULL.

edubart · 2023-11-16T15:11:48Z

linux/libcmt/src/ioctl/rollup-driver.c

+}
+
+int cmt_rollup_driver_fini(struct cmt_rollup_driver *me)
+{


I think all _fini should invalidate all internal structure fields. I know using me after finalizing it is stated as undefined, but we should try to minimize the amount of undefined things that could happen. Leaving this way it could crash or not, cleaning it up it's likely that any subsequent function call will either fail or crash.

edubart · 2023-11-16T15:19:12Z

linux/libcmt/include/libcmt/buf.h

+/** Declare a cmt_buf_t with stack backed memory.
+ * @param [in] N - size in bytes
+ * @note don't port */
+#define CMT_BUF_DECL(S, L) cmt_buf_t S[1] = {{ \


When I run clang-tidy I get this error:

/home/bart/projects/cartesi/machine-emulator-sdk/fs/external/tools/linux/libcmt/tests/abi.c:155:2: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] CMT_BUF_DECL(b, 64); ^ ./include/libcmt/buf.h:26:22: note: expanded from macro 'CMT_BUF_DECL' .end = (S)->begin + L, \ ^

I think these macros may not work as expected everywhere. I would vote to remove them, and make tests, examples and everything use the non macro version.

edubart · 2023-11-16T16:02:19Z

linux/libcmt/src/rollup.c

+{
+	if (!me) return -EINVAL;
+
+	memcpy(me->tx, data, length);


What if length is bigger than tx buffer? Needs bounds checking before copying.

algebraic-dev · 2023-11-29T14:50:10Z

linux/libcmt/include/libcmt/rollup.h

+ * - 0 success
+ * - -ENOBUFS no space left in @p me */
+int
+cmt_rollup_emit_report(cmt_rollup_t *me


I think that the name of this function is wrong. It emits an exception so it probably should be cmt_rollup_emit_exception

What a terrible copy pasta... nice catch.

mock: components can be compiled natively: ioctl: require the cartesi headers and a riscv compiler:

mpolitzer · 2024-01-08T13:56:31Z

migrated to: #29

mpolitzer requested review from edubart, mpernambuco, vfusco, diegonehab, alexmikhalevich and a team October 11, 2023 14:15

mpolitzer self-assigned this Oct 11, 2023

mpolitzer added the enhancement New feature or request label Oct 11, 2023

diegonehab reviewed Oct 12, 2023

View reviewed changes

linux/libdriver/examples/rollup.c Outdated Show resolved Hide resolved

diegonehab reviewed Oct 12, 2023

View reviewed changes

linux/libdriver/base/rollup-driver-api.h Outdated Show resolved Hide resolved

diegonehab reviewed Oct 12, 2023

View reviewed changes

mpolitzer mentioned this pull request Oct 23, 2023

[WIP] feat: add libevm #22

Closed

mpolitzer force-pushed the feature/libdriver branch 3 times, most recently from b1b3059 to 5050981 Compare October 26, 2023 14:44

edubart mentioned this pull request Oct 26, 2023

Rollup simplification cartesi/machine-emulator#137

Closed

16 tasks

edubart requested changes Oct 26, 2023

View reviewed changes

edubart reviewed Nov 3, 2023

View reviewed changes

mpolitzer force-pushed the feature/libdriver branch 4 times, most recently from efc8b50 to af82b44 Compare November 10, 2023 16:12

diegonehab requested changes Nov 10, 2023

View reviewed changes

mpolitzer force-pushed the feature/libdriver branch 4 times, most recently from 078c405 to 573dc5d Compare November 14, 2023 21:32

edubart requested changes Nov 16, 2023

View reviewed changes

edubart reviewed Nov 16, 2023

View reviewed changes

mpolitzer force-pushed the feature/libdriver branch from bf631fc to b12cdda Compare November 16, 2023 19:04

guidanoli mentioned this pull request Nov 17, 2023

feat: blockchain agnostic inputs cartesi/rollups-contracts#89

Merged

mpolitzer force-pushed the feature/libdriver branch from b12cdda to ad2e3eb Compare November 21, 2023 13:59

mpolitzer force-pushed the feature/libdriver branch from ad2e3eb to e26576c Compare November 28, 2023 17:39

algebraic-dev reviewed Nov 29, 2023

View reviewed changes

mpolitzer force-pushed the feature/libdriver branch from d2c9b18 to c3b1bde Compare December 6, 2023 22:33

feat: add libdriver

45510a1

mock: components can be compiled natively: ioctl: require the cartesi headers and a riscv compiler:

mpolitzer force-pushed the feature/libdriver branch from c3b1bde to 45510a1 Compare December 6, 2023 22:35

mpolitzer closed this Jan 8, 2024

mpolitzer deleted the feature/libdriver branch January 8, 2024 14:55

libdriver #24

libdriver #24

Conversation

mpolitzer commented Oct 11, 2023 • edited Loading

THIS IS WORK IN PROGRESS

diegonehab left a comment

Choose a reason for hiding this comment

edubart Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

edubart Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

edubart Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegonehab left a 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

edubart Nov 10, 2023 • 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

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

edubart Nov 16, 2023 • 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

edubart Nov 16, 2023 • 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

algebraic-dev Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpolitzer commented Jan 8, 2024

mpolitzer commented Oct 11, 2023 •

edited

Loading

edubart Oct 26, 2023 •

edited

Loading

edubart Oct 26, 2023 •

edited

Loading

edubart Oct 26, 2023 •

edited

Loading

edubart Nov 10, 2023 •

edited

Loading

edubart Nov 16, 2023 •

edited

Loading

edubart Nov 16, 2023 •

edited

Loading

algebraic-dev Nov 29, 2023 •

edited

Loading