search

fort-nix / nix-bitcoin

A collection of Nix packages and NixOS modules for easily installing full-featured Bitcoin nodes with an emphasis on security.
https://nixbitcoin.org
MIT License
512 stars 108 forks source link

trustedcoin: 0.7.0 -> 0.8.0 #717

Closed seberm closed 4 months ago

seberm commented 4 months ago

CC: @jonasnick @erikarvstedt , should be ready for review

jonasnick commented 4 months ago

Thanks @seberm. I get the following error:

❯ run-tests.sh -s trustedcoin
trustedcoin> Running phase: unpackPhase
trustedcoin> unpacking source archive /nix/store/qyvjv6v77bw72gmq43bxawmgpddz3j4w-source
trustedcoin> source root is source
trustedcoin> Running phase: patchPhase
trustedcoin> Running phase: updateAutotoolsGnuConfigScriptsPhase
trustedcoin> Running phase: configurePhase
trustedcoin> Running phase: buildPhase
trustedcoin> go: go.mod requires go >= 1.22 (running go 1.21.9; GOTOOLCHAIN=local)
erikarvstedt commented 4 months ago

The pkg build works when applied on top of #718 (Update to Nixos 24.05). But the VM test fails:

vm-test-run-nix-bitcoin-trustedcoin> machine # [  207.161568] lightningd[1220]: INFO    lightningd: v24.05
vm-test-run-nix-bitcoin-trustedcoin> machine # [  207.178262] lightningd[1220]: INFO    lightningd: Creating configuration directory /var/lib/clightning/regtest
vm-test-run-nix-bitcoin-trustedcoin> machine # [  207.185605] lightningd[1220]: INFO    plugin-manager: /nix/store/6d7avsmkf26n77mv16ji1ffcibxdbfnd-clightning-24.05/libexec/c-lightning/plugins/bcli: disabled via disable-plugin
vm-test-run-nix-bitcoin-trustedcoin> machine # [  207.189951] lightningd[1220]: INFO    plugin-manager: /nix/store/6d7avsmkf26n77mv16ji1ffcibxdbfnd-clightning-24.05/libexec/c-lightning/plugins/offers: disabled via disable-plugin
vm-test-run-nix-bitcoin-trustedcoin> machine # [  211.108950] lightningd[1220]: INFO    plugin-wss-proxy: Killing plugin: disabled itself: No python3 binary found
vm-test-run-nix-bitcoin-trustedcoin> machine # [  212.942514] lightningd[1220]: INFO    lightningd: Creating database
vm-test-run-nix-bitcoin-trustedcoin> machine # [  214.589195] lightningd[1220]: UNUSUAL hsmd: HSM: created new hsm_secret file
vm-test-run-nix-bitcoin-trustedcoin> machine # [  215.547783] lightningd[1314]: 2024-07-15T18:45:11.509Z plugin-trustedcoin initialized plugin 0.8.0
vm-test-run-nix-bitcoin-trustedcoin> machine # [  215.561709] lightningd[1314]: 2024-07-15T18:45:11.522Z plugin-trustedcoin tip: 0
vm-test-run-nix-bitcoin-trustedcoin> machine # [  215.569149] lightningd[1314]: 2024-07-15T18:45:11.525Z plugin-trustedcoin estimatefees error: none of the esploras returned usable responses
vm-test-run-nix-bitcoin-trustedcoin> machine # [  215.651113] lightningd[1220]: /nix/store/hajxy2q7dan3qd8kzsc6zx2vvvmn7f7q-trustedcoin-0.8.0/bin/trustedcoin error: bad response to estimatefees.feerate_floor (Not a u32?), response was null
vm-test-run-nix-bitcoin-trustedcoin> machine # [  215.707058] systemd[1]: clightning.service: Main process exited, code=exited, status=1/FAILURE
seberm commented 4 months ago

Hello @erikarvstedt , I can confirm I am able to reproduce the error in my testing env. The CLN changed the format of estimatefees, so the trustedcoin moved to a new spec:

The strange error is the missing python3 binary for the wss-proxy plugin: 

plugin-wss-proxy: Killing plugin: disabled itself: No python3 binary found

Support for the wss-proxy plugin was merged in CLN v24.05, and it has its own extra dependencies.

Perhaps we should try to fix this error in the CLN upstream first (@prusnak)? Then, we can add support for the wss-proxy configuration into the clightning.nix module.

I am not sure whether the wss-proxy error could be related to the second error in any way:

trustedcoin error: bad response to estimatefees.feerate_floor (Not a u32?), response was null

@fiatjaf, could you please help investigate whether this might be a bug in trustedcoin?

Thanks

fiatjaf commented 4 months ago

@1ma do you understand this?

fiatjaf commented 4 months ago

Yes, looks like a bug in trustedcoin, I'll try to fix it. It should never be sending null.

fiatjaf commented 4 months ago

Pushed a fix and released as v0.8.1.

What is this plugin-wss-proxy? Is it related?

In any case if there is no bitcoind configured and you can't reach any of the hardcoded Esplora servers everything will crash, there is no other way. Do you have an idea on why all the calls to Esploras failed in the test?

1ma commented 4 months ago

@1ma do you understand this?

Yes, I think the problem is in this line. https://github.com/nbd-wtf/trustedcoin/blob/726f07e6dbf684e0219b6615b4ceb169877ee802/main.go#L136

The plugin returns the serialization of an empty EstimatedFees struct. Since all its fields are *int it's serializing to null values and CLN doesn't understand that.

bcli.c has a predefined response for when it fails to fetch estimates, trustedcoin should also do that. https://github.com/ElementsProject/lightning/blob/1e1d072b65a343d4818e92bb83e2f73b849f050f/plugins/bcli.c#L490

Btw, issue tracked here https://github.com/nbd-wtf/trustedcoin/issues/29

1ma commented 4 months ago

@fiatjaf ok, seen the hotfix. Changing the definition of the struct fields from *int to int might fix this crash but it's still a bit wonky, because now the empty struct serializes to the integer 0. Returning 0 s/kB is still weird.

When fees are not available the plugin should answer {"feerate_floor": 1000, "feerates": []}

1ma commented 4 months ago

@seberm The python3 error is unrelated to the trustedcoin bug. clnrest and wss-proxy are two new official plugins bundled with CLN that are written in Python.

If the Python interpreter is not installed (or the dependencies they need) the plugins don't initialize, but this has nothing to do with trustedcoin, which is a native binary written in Golang.

You can read this Dockerfile to understand how to set up python so that these plugins can run (it only prepares clnrest because I'm not interested in wss-proxy for now, but it's the same idea): https://github.com/1ma/dockertronics/blob/master/core-lightning/debian/Dockerfile

fiatjaf commented 4 months ago

because now the empty struct serializes to the integer 0. Returning 0 s/kB is still weird.

It won't happen because we check first if the values were correctly returned from bitcoind and otherwise fallback to Esplora. Also returning nil is just broken behavior anyway, since lightningd doesn't expect these and will crash.

When fees are not available the plugin should answer {"feerate_floor": 1000, "feerates": []}

No, it should not, returning a known wrong value will just cause channels to close like crazy. Is this weird recommendation written somewhere?

1ma commented 4 months ago

No, it should not, returning a known wrong value will just cause channels to close like crazy. Is this weird recommendation written somewhere?

I understand this is what the normal bcli plugin bundled with CLN does when it cannot fetch estimates. Look for calls to estimatefees_null_response() here, and the function body: https://github.com/ElementsProject/lightning/blob/1e1d072b65a343d4818e92bb83e2f73b849f050f/plugins/bcli.c#L490

1ma commented 4 months ago

because now the empty struct serializes to the integer 0. Returning 0 s/kB is still weird.

It won't happen because we check first if the values were correctly returned from bitcoind and otherwise fallback to Esplora.

The empty EstimatedFees struct in main.go is precisely for the case when the Esplora fetch fails. And I believe Esplora queries are failing more often than they should right now because the getFeeRatesFromEsplora() function is returning too early (when one query fails the loop should keep iterating the remaining endpoints instead of bubble up the error: https://github.com/nbd-wtf/trustedcoin/blob/726f07e6dbf684e0219b6615b4ceb169877ee802/estimatefees.go#L82).

This would be a different bug.

fiatjaf commented 4 months ago

And I believe Esplora queries are failing more often than they should right now because the getFeeRatesFromEsplora() function is returning too early (when one query fails the loop should keep iterating the remaining endpoints instead of bubble up the error

Nice catch, I've pushed a fix, but the error on the log above was different, it was the error message that happens when the full loop has been completed.

seberm commented 4 months ago

Hello @fiatjaf , could you please push the new trustedcoin release tags? From your commits I suppose you have released v0.8.1 and v0.8.2, but the tags are not there:

Thanks!

fiatjaf commented 4 months ago

Oh, sorry, I thought I had done that.

seberm commented 4 months ago

Hello @erikarvstedt , to problem seems to be fixed with trustedcoin v0.8.2. Could you please retest?

vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.581307] lightningd[1032]: UNUSUAL hsmd: HSM: created new hsm_secret file                                                                                                                
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.653858] lightningd[1085]: 2024-07-24T21:33:54.787Z plugin-trustedcoin initialized plugin 0.8.2                                                                                          
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.675766] lightningd[1085]: 2024-07-24T21:33:54.810Z plugin-trustedcoin bitcoind RPC working, will use that with highest priority and fall back to block explorers if it fails.           
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.676192] lightningd[1085]: 2024-07-24T21:33:54.810Z plugin-trustedcoin tip: 100                                                                                                          
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.686627] lightningd[1085]: 2024-07-24T21:33:54.821Z plugin-trustedcoin estimatefees error: none of the esploras returned usable responses                                                
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.705184] lightningd[1085]: 2024-07-24T21:33:54.839Z plugin-trustedcoin returning block 70, 38f436db7ddf470032f34152be…, 249 bytes                                                        
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.783965] lightningd[1032]: INFO    plugin-chanbackup: Creating Emergency Recovery                                                                                                        
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.802452] lightningd[1032]: INFO    plugin-bookkeeper: Creating database                                                                                                                  
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.805403] lightningd[1032]: INFO    lightningd: --------------------------------------------------                                                                                        
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.805638] lightningd[1032]: INFO    lightningd: Server started with public key 03af71d3ca9605735ae9424d35352b87f4d3b6fa28ce847d14958123fea3c51d78, alias UNITEDPLOW (color #03af71) and li
ghtningd v24.05                                                                                                                                                                                                                               
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.815740] lightningd[1085]: 2024-07-24T21:33:54.950Z plugin-trustedcoin returning block 71, 132efb0cee1befd2aa36ae487f…, 249 bytes                                                        
vm-test-run-nix-bitcoin-trustedcoin> machine # [   22.837524] lightningd[1085]: 2024-07-24T21:33:54.972Z plugin-trustedcoin returning block 72, 1f0ec50c3037cbedd77d634f56…, 249 bytes

Thanks.