Update `make` based instructions

emlautarom1 commented 1 week ago

Current Behavior

make start-operator errors out immediately with the following error:

{"err":"open tests/keys/bls/1.bls.key.json: no such file or directory"}

More details on #257

New Behavior

This PR updates make based instructions to match the current Docker setup.

Breaking Changes

None beside getting something to work that was previously broken.

Observations

Added a new near-da-rpc-sys make command that builds the native libraries required by the relayer.
Adjusted the CGO linker flags so the relayer can pick the required native libraries during build time.
Updated README with instructions on how to build the native libraries.
Updated config-files/operator.anvil.yaml to match the latest file structure regarding keys.

firatsertgoz commented 1 week ago

May I suggest another angle,

How about instead of using da-rpc-sys you straight up use the http-api. Yes you would need to run a sidecar with the relayer, but the seperation would be very clean.

taco-paco commented 1 week ago

May I suggest another angle,

How about instead of using da-rpc-sys you straight up use the http-api. Yes you would need to run a sidecar with the relayer, but the seperation would be very clean.

That is a great idea. @emlautarom1 do you want to play with it? This would require small Rust service on top of rollup-data-availability.

firatsertgoz commented 1 week ago

May I suggest another angle, How about instead of using da-rpc-sys you straight up use the http-api. Yes you would need to run a sidecar with the relayer, but the seperation would be very clean.

That is a great idea. @emlautarom1 do you want to play with it? This would require small Rust service on top of rollup-data-availability.

We already have a service for this in the repo! https://github.com/Nuffle-Labs/data-availability/blob/main/bin/http-api/src/main.rs Just need to run the binary at the same time with the relayer, POST and GET calls through relayer in Go. No need to do anything in Rust :)

emlautarom1 commented 1 week ago

I would propose to refrain from getting rollup-data-availability as submodule.

We shall list rollup-data-availability artifacts as prerequisites. Maybe with explanation on how to compile it. User shall invoke make command here and then move it to /usr/local/lib. Go will pick it up itself after that

Initially I worked on a different approach that relied on cloning the rollup-data-availability repo, run the appropriate make command, copied the artifacts and then deleted the repo. This removes the need for the submodule and ensures that we're building for the correct platform, without introducing additional friction.

I'm against installing the libraries globally (ex. /usr/local/lib), so we would like to "install" them locally (this PR uses relayer/libs). The issue is then how do we ensure that the Go compiler includes that path during the linking phase without polluting the global defaults.

I think defining the make command to something like CGO_LDFLAGS="-L ./relayer/libs" go run ... should be enough.

The compilation of relayer is platform-dependant and rollup-data-availability handles that here and here.

My understanding is that our relayer (relayer/) is not platform dependent but rather it has a dependency on native libraries. What we need to do is to point Go to the correct directory while compiling the relayer to properly pick up these libraries.

emlautarom1 commented 1 week ago

Regarding the alternative proposed by @firatsertgoz, I think it would be nice to explore it later. For now I would like to fix the current make based instructions without adding additional dependencies, in particular considering that the docker-compose workflow is already working as intended (and it uses da-rpc-sys under the hood).

Hyodar commented 4 days ago

About the Go sidecar conversation - I agree with both. We should move to the Go sidecar but IMO this PR should keep its scope.

Reason for that is already in #165. We can move ahead with it, but I'd still prefer if it kept a compatibility layer in this case. AFAIK we don't have other breaking updates piled up and coordinating those should be very costly - there are nodes from all over the world and some are more responsive than others.

taco-paco commented 4 days ago

@Hyodar thanks for implicitly reminding about the PR) With regards to sidecar we can just use the current commit of near-da, thus it shall be only relayer update. After we would probably be able to discard #165 and just update the sidecar along with operator.

@emlautarom1

I'm against installing the libraries globally (ex. /usr/local/lib), so we would like to "install" them locally (this PR uses relayer/libs). The issue is then how do we ensure that the Go compiler includes that path during the linking phase without polluting the global defaults.

The thing is that CGO_LDFLAGS shall be customizable by user himself not by us. There're multiple C libraries that our code depends on implicitly handled by go itself we can't go with the way of trying to specify them for users. If user has custom locations with zlib, glibc, rollup-da he can use CGO_LDFLAGS himself.

Unfortunately there's no such thing as build.rs in GO

Hyodar commented 3 days ago

Okay, all conversation aside, I think the first question we should answer here is whether this tooling is important atm. It was used before and increasingly unused over time. Are we using these individual commands, or should we just deprecate this and use exclusively docker compose containers? On my end, I usually just use the containers directly, as it's actually closer to the real scenario.

emlautarom1 commented 3 days ago

Are we using these individual commands, or should we just deprecate this and use exclusively docker compose containers? On my end, I usually just use the containers directly, as it's actually closer to the real scenario.

I favor deprecating the make based instructions and only use docker as the unique workflow (less maintenance burden) but that's outside the scope of this PR. Note that we should still have make for development purposes (ex. run tests, build images, etc.)

Hyodar commented 3 days ago

Okay, after checking the whole conversation again, we can generally move forward with this. My overall take is:

Depending on a global library to be managed by the user is not nice as this is a quite specific application. We should try to keep things local as much as possible.
This is going to be deprecated (really) soon, I don't think we should spend more headspace with this

That said, I understand the trouble here, but overall I think defining the library as it is the best approach for now, though I'd highly prefer if we concatted (e.g. CGO_LDFLAGS += -L/path/to/your/library) instead of redefining the env variable. This way the user is not tied to our definition. The compilation instructions should also be updated in the docs.

Again, this should be deprecated really soon in favor of only container-based testing, so we don't have much to gain on this discussion.

emlautarom1 commented 2 days ago

That said, I understand the trouble here, but overall I think defining the library as it is the best approach for now, though I'd highly prefer if we concatted (e.g. CGO_LDFLAGS += -L/path/to/your/library) instead of redefining the env variable. This way the user is not tied to our definition

Done, now we concatenate existing CGO_LDFLAGS if any, allowing the user to configure this behavior if needed.

The compilation instructions should also be updated in the docs.

Already updated the main README.md with new instructions as described in the PR body.

Hyodar commented 2 days ago

Added the deprecation of the non-container testing environment instructions as #269.

emlautarom1 commented 1 day ago

Merging as discussed during today's daily

NethermindEth / near-sffl