Skip to content

fix(electrum): query block count in sequential manner#2666

Merged
onur-ozkan merged 2 commits intodevfrom
many-electrum-conns
Oct 27, 2025
Merged

fix(electrum): query block count in sequential manner#2666
onur-ozkan merged 2 commits intodevfrom
many-electrum-conns

Conversation

@mariocynicys
Copy link
Copy Markdown
Collaborator

This PR makes it so we query electrums latest blocks in sequential manner instead of in parallel. This guarantees that we don't break out of max_connected + 1 (note the +1 here is because such a query is a forced connection and background task used for managing connections might already have max_connected connections that are different from the forced connection).

This call was only used in block_header_utxo_loop which is only performed for lightning with spv_conf (but i cant find it in coins file).

This makes querying latest block a bit slower, but we don't need it to be fast anyway.

as in, perfrom them in series instead of in parallel (and label the connection as not_needed once done with the request).
this makes it so we won't break the max_connected threshold (this is a lie as we can break it by at most one extra connection due to the running background task that also manages connections aside from us forcefully instantiating connections in get_servers_with_latest_block_count)
since any forceful request will always call not_needed() at the end (check ElectrumClient::electurm_request_to())
@dimxy
Copy link
Copy Markdown
Collaborator

dimxy commented Oct 27, 2025

In this PR we select servers returning max height. Are we sure this is a correct way to do this?
Servers on fork may have different heights, even greater than the height in the actual network.

@mariocynicys
Copy link
Copy Markdown
Collaborator Author

In this PR we select servers returning max height. Are we sure this is a correct way to do this?
Servers on fork may have different heights, even greater than the height in the actual network.

I would address that in another PR (if it's a problem), but let's have a discussion here about that nonetheless.

You here mean that there might be a longer chain but with less work right?

@dimxy
Copy link
Copy Markdown
Collaborator

dimxy commented Oct 27, 2025

In this PR we select servers returning max height. Are we sure this is a correct way to do this?
Servers on fork may have different heights, even greater than the height in the actual network.

I would address that in another PR (if it's a problem), but let's have a discussion here about that nonetheless.

You here mean that there might be a longer chain but with less work right?

yes exactly, a forked node can longer chain with less work

Copy link
Copy Markdown

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

@onur-ozkan onur-ozkan merged commit c1175ce into dev Oct 27, 2025
22 of 28 checks passed
@onur-ozkan onur-ozkan deleted the many-electrum-conns branch October 27, 2025 16:23
@mariocynicys
Copy link
Copy Markdown
Collaborator Author

@dimxy aha got you. we def need to compare chain works instead of max height. i will create an issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants