Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error "non-final" when broadcasting #8245

Closed
bar-boss opened this issue Mar 15, 2023 · 8 comments
Closed

Error "non-final" when broadcasting #8245

bar-boss opened this issue Mar 15, 2023 · 8 comments
Labels
bug 🐞 CLI/RPC ▶ topic-network 🕸 related to logic in network.py (etc) topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py
Milestone

Comments

@bar-boss
Copy link

Ubuntu, PHP. Use Electrum via JSON RPC.
Method "broadcast"
Sometimes I get an error: {"id": "curltext", "jsonrpc": "2.0", "error": {"code": 1, "message": "non-final"}}

What mean this error? 6 months work all good and never get this error. But now I get this error near 0.5% off all my send transactions.

@SomberNight
Copy link
Member

To confirm, are you calling the "broadcast" command of electrum, via RPC, when getting that error?

Can you share the raw tx?

non-final comes from bitcoind I think, and it probably suggests either the locktime is in the future, or an incorrectly set nsequence

@ecdsa
Copy link
Member

ecdsa commented Mar 18, 2023

Maybe the server is lagging? we use chain.height() in get_locktime_for_new_transaction.
I can imagine how a server that is only a bit slow could be one block behind others 0.5% of the time.

We could use the server's height instead of chain.height, or we could broadcast to one of the connected nodes that has the same height as our chain. With the first approach, a server can trick us to create transactions with arbitrarily low locktimes, so maybe the delta should be capped. With the second approach there is a loss of privacy, because we are no longer using nodes for headers only.

@SomberNight SomberNight added bug 🐞 topic-network 🕸 related to logic in network.py (etc) topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py labels Mar 18, 2023
@SomberNight
Copy link
Member

Ah. Great point!
Indeed that would explain it.

I suggest using min(chain.height(), network.get_server_height()).
With auto_connect=false, we should definitely only use the main server to broadcast, and we can't switch away anyway. The GUI shows if the server is lagging, and the user can take manual action if they want. This lets automated systems keep working too.
With auto_connect=true, if the server is lagging too much (2 blocks), the network will switch to a different server anyway.

So I think we don't need to introduce the extra complication of broadcasting to other servers, just for this.

diff --git a/electrum/wallet.py b/electrum/wallet.py
index b0db8b3e43..28c369fb6a 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -208,11 +208,17 @@ def get_locktime_for_new_transaction(network: 'Network') -> int:
     if chain.is_tip_stale():
         return 0
     # discourage "fee sniping"
-    locktime = chain.height()
+    # note: main server might be lagging
+    #       (but if it's lagging too much, it is the network's job to switch away)
+    locktime = min(
+        chain.height(),  # learnt from all connected servers, SPV-checked
+        network.get_server_height(),  # height claimed by main server, unverified
+    )
     # sometimes pick locktime a bit further back, to help privacy
     # of setups that need more time (offline/multisig/coinjoin/...)
     if random.randint(0, 9) == 0:
         locktime = max(0, locktime - random.randint(0, 99))
+    locktime = max(0, locktime)
     return locktime

@SomberNight SomberNight added this to the 4.4.0 milestone Mar 18, 2023
@bar-boss
Copy link
Author

To confirm, are you calling the "broadcast" command of electrum, via RPC, when getting that error?

Can you share the raw tx?

non-final comes from bitcoind I think, and it probably suggests either the locktime is in the future, or an incorrectly set nsequence

Yes, I use broadcast method.

$postfields['method'] = "broadcast"; $postfields['params']['tx'] = $tx; $ch = curl_init(); curl_setopt($ch, CURLOPT_URL, 'http://'.$this -> electrum_username.':'.$this -> electrum_password.'@'.$this -> electrum_host.':'.$this -> electrum_port); curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1); curl_setopt($ch, CURLOPT_POST, 1); curl_setopt($ch, CURLOPT_POSTFIELDS, json_encode($postfields)); $result = curl_exec($ch); curl_close($ch); var_dump($result);

Raw tx unfortunately I can't find it right now

@bar-boss
Copy link
Author

Maybe the server is lagging? we use chain.height() in get_locktime_for_new_transaction. I can imagine how a server that is only a bit slow could be one block behind others 0.5% of the time.

We could use the server's height instead of chain.height, or we could broadcast to one of the connected nodes that has the same height as our chain. With the first approach, a server can trick us to create transactions with arbitrarily low locktimes, so maybe the delta should be capped. With the second approach there is a loss of privacy, because we are no longer using nodes for headers only.

We have several servers where electrum is installed. This error is observed on half of them.

@SomberNight
Copy link
Member

By "server", the electrum server that the client is connected to was meant; not the machine running the client.

@bar-boss
Copy link
Author

By "server", the electrum server that the client is connected to was meant; not the machine running the client.

I understand that you changed the algorithm and this error will not be from version 4.4.0?

@SomberNight
Copy link
Member

yes, should be fixed on master now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 CLI/RPC ▶ topic-network 🕸 related to logic in network.py (etc) topic-wallet 👛 related to wallet.py, or maybe address_synchronizer.py/coinchooser.py
Projects
None yet
Development

No branches or pull requests

3 participants