serai-dex / serai

Other
266 stars 49 forks source link

Check if wasm was built by container exit code and state instead of local mountpoint #570

Closed rlking closed 6 months ago

kayabaNerve commented 6 months ago

ACK to first file, and concept ACK to logging. Logging should be via log though, not println. I'm happy to make that change myself if you'd prefer (I don't want to bog you down).

Thanks for doing this :) This def was an annoyance last time around.

rlking commented 6 months ago

Thanks for the input, I'll check how to use the logger. Atm it looks super weird when just printing the path without the timestamp like the rest of the runtime

rlking commented 6 months ago

Ok I need to pass on this simple task of adding a logger to the node. What I tried was adding the log dependency and then the simple logger implementation and tried to init it in the main method, but it always crashes with "Error: GlobalLoggerError(SetLoggerError(SetLoggerError(())))" then.

I reverted the log line, the important thing is to not get stuck when building the wasm, because I also have this issue when running on windows with a clean docker desktop installation.

kayabaNerve commented 6 months ago

Ok I need to pass on this simple task of adding a logger to the node.

That'd explain your issue :) You don't need to add a logger. You just need to invoke log::info. Substrate already instantiated a logger and registered it globally (which is why your logger being registered is erroring).

kayabaNerve commented 6 months ago

Sorry it was a bother for you, and definitely agreed on the priority being this (which I appreciate you picking up).

Will merge once I locally test :+1:

kayabaNerve commented 6 months ago

Thank you!

rlking commented 6 months ago

Hmm, regarding the logger do you see the log line on startup? Because I don't get the log at all. I first had it like this, but since it didn't print anything I thought of initializing the logger. Once I did that, I saw the log but then probably someone else also tries to init it and then it crashes. I did a cargo tree to maybe see who is adding a logger implementation but I only found everyone is using log. So maybe substrate does it and we are just to early.

kayabaNerve commented 6 months ago

I noticed it didn't print after I made the commit, while doing local testing. Immediately, I assumed it was broken. It may not print due to the fact the node panics a moment later (and parity may have implemented their logger in an asynchronous fashion)? But I don't care to say that with confidence.

I could either delete the attempt which at least may work later, continue holding up this PR over this (obviously not worth it), or just leave it in to review at said later time (though by that logic I shouldn't have nit'd your println, though to be fair, I didn't predict log wouldn't work at the time).

Sorry for potentially gas-lighting you on "You just need to invoke log::info" (obviously not quite that simple :sweat_smile:), and also for not documenting my commit's questionable nature. I just truly didn't care to further hold this up. When local tests passed, barring that, I pushed the typo I inadvertently introduced and moved on.


I did take a moment to review.

2024-05-26T01:55:39.8085756Z 2024-05-26 01:27:56 Serai Node    
2024-05-26T01:55:39.8087001Z 2024-05-26 01:27:56 ✌️  version 0.1.0-unknown    
2024-05-26T01:55:39.8088307Z 2024-05-26 01:27:56 ❤️  by Luke Parker <lukeparker5132@gmail.com>, 2022-2024    
2024-05-26T01:55:39.8089606Z 2024-05-26 01:27:56 📋 Chain specification: Local Test Network    
2024-05-26T01:55:39.8090560Z 2024-05-26 01:27:56 🏷  Node name: Alice    
2024-05-26T01:55:39.8091277Z 2024-05-26 01:27:56 👤 Role: AUTHORITY    
2024-05-26T01:55:39.8092517Z 2024-05-26 01:27:56 💾 Database: RocksDb at /home/serai/.local/share/serai-node/chains/local/db/full    
2024-05-26T01:55:39.8094387Z 2024-05-26 01:27:57 🔨 Initializing Genesis block/state (state: 0xf3bc…6934, header-hash: 0x6d30…297f)    
2024-05-26T01:55:39.8096046Z 2024-05-26 01:27:57 👴 Loading GRANDPA authority set from genesis on what appears to be first startup.    
2024-05-26T01:55:39.8097645Z 2024-05-26 01:27:59 👶 Creating empty BABE epoch changes on what appears to be first startup.    
2024-05-26T01:55:39.8099358Z 2024-05-26 01:27:59 🏷  Local node identity is: 12D3KooWQkaHG9T7jCUVfVgJQzxa4fqeyWGLBv1oYAGUokxHvcJa    
2024-05-26T01:55:39.8101035Z 2024-05-26 01:27:59 Local peer id: 12D3KooWQkaHG9T7jCUVfVgJQzxa4fqeyWGLBv1oYAGUokxHvcJa    
2024-05-26T01:55:39.8102216Z 2024-05-26 01:27:59 💻 Operating system: linux    
2024-05-26T01:55:39.8103020Z 2024-05-26 01:27:59 💻 CPU architecture: x86_64    
2024-05-26T01:55:39.8103807Z 2024-05-26 01:27:59 💻 Target environment: gnu    
2024-05-26T01:55:39.8104714Z 2024-05-26 01:27:59 💻 CPU: AMD EPYC 7763 64-Core Processor    
2024-05-26T01:55:39.8105513Z 2024-05-26 01:27:59 💻 CPU cores: 2    
2024-05-26T01:55:39.8106338Z 2024-05-26 01:27:59 💻 Memory: 15981MB    
2024-05-26T01:55:39.8107055Z 2024-05-26 01:27:59 💻 Kernel: 6.5.0-1021-azure    
2024-05-26T01:55:39.8108070Z 2024-05-26 01:27:59 💻 Linux distribution: Debian GNU/Linux 12 (bookworm)    
2024-05-26T01:55:39.8109162Z 2024-05-26 01:27:59 💻 Virtual machine: yes    
2024-05-26T01:55:39.8110157Z 2024-05-26 01:27:59 📦 Highest known block at #0    
2024-05-26T01:55:39.8111169Z 2024-05-26 01:27:59 〽️ Prometheus exporter started at 127.0.0.1:9615    
2024-05-26T01:55:39.8112436Z 2024-05-26 01:27:59 Running JSON-RPC server: addr=0.0.0.0:9944, allowed origins=["*"]    
2024-05-26T01:55:39.8113610Z 2024-05-26 01:27:59 👶 Starting BABE Authorship worker    
2024-05-26T01:55:39.8115039Z 2024-05-26 01:28:00 discovered: 12D3KooWE6j6ZjcBePHoWZXsuuAJs4ZmDDksc4wSfxaqsgrDbfGT /ip4/172.18.0.8/tcp/30333    
2024-05-26T01:55:39.8116137Z 2024-05-26 01:28:02 discovered: 12D3KooWKFFobbUFMDBmjjCsLa39xj7RHjucpKQfeWMR5j6hmbAQ /ip4/172.18.0.15/tcp/30333    
2024-05-26T01:55:39.8117206Z 2024-05-26 01:28:04 💤 Idle (2 peers), best: #0 (0x6d30…297f), finalized #0 (0x6d30…297f), ⬇ 1.7kiB/s ⬇ 1.8kiB/s    
2024-05-26T01:55:39.8117904Z 2024-05-26 01:28:04 Accepting new connection 1/100
2024-05-26T01:55:39.8118360Z 2024-05-26 01:28:04 Accepting new connection 2/100
2024-05-26T01:55:39.8119137Z 2024-05-26 01:28:05 discovered: 12D3KooWQLFnvFaHubrqrwn5WuC7hx4VnKCF6J1qM8Th2uvdvUC1 /ip4/172.18.0.24/tcp/30333    
2024-05-26T01:55:39.8120029Z 2024-05-26 01:28:05 Accepting new connection 3/100
2024-05-26T01:55:39.8120873Z 2024-05-26 01:28:06 👶 New epoch 0 launching at block 0xedf8…91fe (block slot 286114481 >= start slot 286114481).    
2024-05-26T01:55:39.8121643Z 2024-05-26 01:28:06 👶 Next epoch starts at slot 286517681    
2024-05-26T01:55:39.8122144Z 2024-05-26 01:28:06 Imported #1 (0xedf8…91fe)    

Log isn't present on a node which doesn't panic (a devnet node. I made my initial thoughts it may be due to the panicking premised due to trying a testnet build (which does panic), as that'd test the testnet wasm building + build check, the entire point of this PR. That means yeah, no, this log line just isn't valid here :/ I'll try to follow up there.

kayabaNerve commented 6 months ago

FWIW, we're not insane. Those initial lines are using log::info. I have no idea why this log::info (which yes, should be proper) isn't so working.

rlking commented 6 months ago

Sorry for potentially gas-lighting you on "You just need to invoke log::info" (obviously not quite that simple 😅)

Haha no worries. We need to follow some simple best practices and it is a complete classic that the first and most simple thing makes troubles 😄

To find some peace of mind I debugged into it and the logger is initialized right after calling load_spec, so a little to late for us :/ https://github.com/serai-dex/substrate/blob/serai/client/cli/src/lib.rs#L210

kayabaNerve commented 6 months ago

I'll look at moving it.