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

Satoshi notifications #12400

Merged
merged 31 commits into from
Apr 19, 2024
Merged

Satoshi notifications #12400

merged 31 commits into from
Apr 19, 2024

Conversation

lontivero
Copy link
Collaborator

@lontivero lontivero commented Feb 8, 2024

Push notifications for Satoshi identity using web sockets.

This PR is for the coordinator to send notifications about changes such as mining fee rates, exchange rates, new filters, round state changes, etc to all the clients. In this way clients don't need to request information using long pooling, instead the simple listen passively and get notifications about different events as soon as those occurs.

Websocket infrastructure

The new server-side infrastructure components are mostly in the Backend's middleware folder. The solution is implemented as a ASP.NET middleware and it looks pretty much as a socket server implementation were after receiving a http request, if it is a protocol upgrade (websocket) request it accepts the connection, register the websocket in a list of websockets and start listening for messages from the connected client. The implementation is pretty generic because the idea is to have a reusable infra for implementing websocket communication for other identities too (Alice is the best candidate here)

Satoshi messages

All what can be known by satoshi is implemented by the SatoshiWebSocketHandler handler. This sends mining fee rates, exchange rates and compact filters as soon as they are created.

Upon establishing the connection, the client transmits the hash of the most recent known block to the coordinator, enabling the latter to start sending filters from that specific hash onwards. After all filters have been sent, it subscribes the socket to receive all new filters.


Note: Continuing with this PR requires lots of changes in many places and for that reason I think it will be better to start changing small things before even doing any further progress.

Fixes: #12117

@lontivero
Copy link
Collaborator Author

Recommendation for reviewers:

  • Test first: Run bitcoind, the coordinator and the client all in regtest.
    • Backend status
    • All fee estimations
    • Mining fee rate
    • Compact filter (synchronization and new generated filters)
  • Review commit by commit first. Some commits were completely removed (ex: 18455ef) however that let you see the design process.
  • Review each topic in isolation, this is, review how fee estimations work now from end to end, review how mining fee estimations work now from end to end, review how wallet synchronize now from end to end, etc.

@Szpoti

This comment was marked as resolved.

@lontivero
Copy link
Collaborator Author

@turbolay @Szpoti any progress here?

@Szpoti
Copy link
Collaborator

Szpoti commented Mar 20, 2024

@turbolay created a test server where we can properly test this, so I'll do live testing and see if it all goes well.

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

Partial review, sorry for the delay, last week I couldn't and Monday I had an issue while setting up my test server

WalletWasabi/Services/SatoshiSynchronizer.cs Show resolved Hide resolved
WalletWasabi/Services/SatoshiSynchronizer.cs Show resolved Hide resolved
}
}

async Task RewindAsync(int count)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this tries to remove more filters than what we have, IndexStore throws and the client is bricked (doesn't try to connect to backend anymore)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. I will take a look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Waiting for #12778

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

This is my final message of the review.
BTW the code is much cleaner than before.

BlockHeight,
ExchangeRate,
MiningFeeRates
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this in a file named FilterMessage? Confusing

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will live with it

@turbolay
Copy link
Collaborator

turbolay commented Apr 8, 2024

I pushed some commits to fix the issues I found.
Commits are easy.
After #12778 merged, this PR should be ready to merged, I tested it again and didn't find additional issues

@Szpoti
Copy link
Collaborator

Szpoti commented Apr 8, 2024

I tested it 2 weeks ago when regtest server was set, didn't find any problems or errors, will do another round of testing with the new changes.

@molnard
Copy link
Collaborator

molnard commented Apr 8, 2024

At the CODE team meeting, we agreed to have this after the release of v2.0.7.

@lontivero
Copy link
Collaborator Author

@turbolay I have rebased this branch on top of the released version and I have removed the merges and the commit fixing merging problems and so on. The idea is that the branch it already too big and I would like to keep it clean.

@turbolay @Szpoti if this PR can be merged please approve it, otherwise let me know what is missing.

turbolay
turbolay previously approved these changes Apr 17, 2024
Copy link
Collaborator

@Szpoti Szpoti left a comment

Choose a reason for hiding this comment

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

Just these small nits, otherwise tested and works.

@lontivero lontivero merged commit 79b8cf7 into WalletWasabi:master Apr 19, 2024
7 of 8 checks passed
lontivero added a commit to lontivero/WalletWasabi that referenced this pull request Apr 28, 2024
lontivero added a commit that referenced this pull request Apr 29, 2024
* Revert "Notify `SoftwareVersion` and `LegalDocumentVersion` (#12692)"

This reverts commit f8305d2.

* Revert "Fix reorg error log (#12925)"

This reverts commit 449a933.

* Revert "Satoshi notifications (#12400)"

This reverts commit 79b8cf7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filters and status messages via web sockets
4 participants