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

Feature request - incoming transaction notifications #778

Open
szollo opened this issue Dec 13, 2020 · 13 comments
Open

Feature request - incoming transaction notifications #778

szollo opened this issue Dec 13, 2020 · 13 comments
Labels
enhancement New feature or request

Comments

@szollo
Copy link
Contributor

szollo commented Dec 13, 2020

It would be amazing to have support for desktop notifications of incoming transactions, so that the user is informed when there's new incoming bitcoin without needing to refresh the browser, or indeed even look at the web interface - this is pretty much the only feature preventing me to fully switch from Electrum at this point.

Taking a cursory look this could be achieved by using ZMQ subscriptions, however as it's not plug-and-play it would probably have to be an advanced setting, as Bitcoin Core default config doesn't enable it (though it's compiled into the official binaries afaik).

The other challenges I can think of, off the top of my head:

  • Having cross-platform desktop notification (perhaps just browser notifications instead of OS native would suffice?)
  • Extra configuration steps if Specter is connecting to the node via Tor, seeing as the ZMQ port would also need to be published as a separate hidden service
  • There's probably a client / frontend websocket component to develop?

Thanks for the great work, Specter is awesome.

@k9ert k9ert added the enhancement New feature or request label Dec 22, 2020
@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Feb 25, 2022

I thought the enhancement is a good idea and developed a proof-of-concept for it: https://github.com/cryptoadvance/specter-desktop/compare/master...relativisticelectron:notification?expand=1

It uses https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API/Using_the_Notifications_API
There is a simple javascript checker (setInterval), checking for updated transactions.
Peek 2022-02-26 09-56

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Feb 26, 2022

@moneymanolis : Do you think this web-push notification would be useful for many users? Or are most users running bitcoin-qt locally, and have therefore the notifications from bitcoin-qt?
@szollo : Is this web-push notification what you had in mind?

If there is demand for this feature I can also check out https://bitcoindev.network/accessing-bitcoins-zeromq-interface/ . Something like js <--> python (currently via setInterval and GET), and python <--> Bitcoin-core (zqm, currently rpc)

@szollo
Copy link
Contributor Author

szollo commented Feb 26, 2022

That's exactly what I had in mind @relativisticelectron - looks great!

@moneymanolis
Copy link
Collaborator

@relativisticelectron looks cool! As for your question about bitcoin-qt, I think nobody uses this, users have usually different variations of bitcoind instances running, either locally or on a remote node.

Tagging @k9ert for architectural input.

Sth. that directly caught my eye from the MDN documentation:

image

Don't we have a user group that struggles with not being able to connect via HTTPS with their (remote) node? I think Umbrel, no?

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Mar 3, 2022

image Don't we have a user group that struggles with not being able to connect via HTTPS with their (remote) node? I think Umbrel, no?

Yes, I have no idea how to deal with this. It seems https is not coming to umbrel any time soon: getumbrel/umbrel-os#251

It works on localhost though...

@k9ert
Copy link
Collaborator

k9ert commented Apr 2, 2022

Wow, this really looks cool. Somehow overlooked this issue. I think the zeromq thing should be more in focus than the frontend-implementation as otherwise, you just create unnecessary core-calls which might cause unnecessary loads on small raspis.

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Apr 15, 2022

@k9ert : Thanks for the hint. A zmq client (proof of concept) turned out to be super easy in python. I added zmq such that rpc calls for notifications are ONLY done if the zmq has gotten a message: https://github.com/cryptoadvance/specter-desktop/compare/master...relativisticelectron:notification?expand=1

  1. The problem with zmq is, that the user has to enable it in bitcoin.conf. Is there a way to change the bitcoin.conf of the bitcoin node that is bundled with specter?
  2. Is there a way to make the javascript <--> python connection more efficient? How to listen to a server side event with javascript? I have now (in this branch) instant push notifications using pywebpush, though it only works on Chome/Chromium... Firefox gives the error
    image . So perhaps the simple setInterval checker is better, because it works in Firefox too...

@k9ert
Copy link
Collaborator

k9ert commented Apr 20, 2022

Wow, this really looks promising and also easier than i thought. When i get that right, you don't clear the buffer to the server-side data-structures directly but just create the notification on the client-side (via continuous pulling and maybe later via web-push) and the then the user is encouraged to make a normal request requests the data. We probably need to also put some logic in for the case: It should profit also via not asking core that often anymore but all of that can wait for now.

Not sure how complex the web-push is but maybe we can separate the two things? I'd probably hesitate to merge web-push if it's not working on firefox but who cares for cheap and open small polls?!

I just had a look in the internal node implementation. Indeed, we have a bitcoin.conf. If we modify that one, we would need a SpecterMigration. But i don't think it's necessary. I don't think we should have created a bitcoin.conf in the first place and another alternative is to "add" the zmq-config via the commandline here:
https://github.com/cryptoadvance/specter-desktop/blob/master/src/cryptoadvance/specter/process_controller/node_controller.py#L333

Can you try to add the necessary ZMQ-stuff into the commandline? Maybe create an extra config.py config value which makes it possible to deactivate ZMQ. IMHO, the commandline options come on top of the bitcoin.conf.

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Apr 20, 2022

Thanks for the feedback!

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Apr 20, 2022

Javascript Notifications

If the permission cannot be granted (because of https missing), and is not denied I can at least show javascript notifications:
Peek 2022-04-20 21-52

@szollo : Would these Javascript notifications (in the browser window) be helpful if the Notifications_API (docs) is not available?

@szollo
Copy link
Contributor Author

szollo commented Apr 21, 2022

That looks great @relativisticelectron!

@szollo : Would these Javascript notifications (in the browser window) be helpful if the Notifications_API (docs) is not available?

For the use case I had in mind I'd prefer it was native to the OS (ie. macOS or Windows notifications) or browser, as more often than not I don't view the active window, especially when travelling and using one screen.

That being said, it's great to have the choice - so if I wanted to have that feature I'd have to make sure I run Specter as an HTTPS service with a trusted certificate?

@relativisticelectron
Copy link
Collaborator

relativisticelectron commented Apr 21, 2022

For the use case I had in mind I'd prefer it was native to the OS (ie. macOS or Windows notifications) or browser, as more often than not I don't view the active window, especially when travelling and using one screen.

Sure, that is best. Without https this Notifications_API (docs) (remotely) is just not possible. I still have to investigate if the push service (google or mozilla) works without https, but it would involve a cloud server that forwards encrypted messages.
TODO:

That being said, it's great to have the choice - so if I wanted to have that feature I'd have to make sure I run Specter as an HTTPS service with a trusted certificate?

  1. Notifications_API (docs) on Localhost doesn't require a certificate.
  2. Yes for remote specter, Notifications_API (docs) does require a certificate. But since this is probably not available in most node packages, maybe javascript notifications are better than no notifications at all? Maybe the website tab title changing from image to "(1) Specter Desktop"?
  3. Notifications_API (docs) seems to work well in the tor browser :-)

Bottom line

  • Notifications_API (docs) for specter on localhost http or tor browser
  • Javascript Notifications for remote http specter
  • Basic principle: javascript setInterval checks for new tx in buffered_zmq_messages <--> python zmq client stores zmq tx messages in buffered_zmq_messages <-- bitcoin zmq server broadcasts zmq tx messages

I that OK for everyone @k9ert , @moneymanolis @szollo ? Should I (slowly) develop this proof-of-concept into a PR?

relativisticelectron added a commit to relativisticelectron/specter-desktop that referenced this issue May 4, 2022
- Added libzmq to build process, to enable future use cases such as cryptoadvance#778
- added --without-gui to remove configure warning that gui will not be built.
@k9ert
Copy link
Collaborator

k9ert commented Jun 5, 2022

Again missed your comment even though in the meantime i'm indeed using the github-task/notification system.

I that OK for everyone @k9ert , @moneymanolis @szollo ? Should I (slowly) develop this [proof-of-concept]
(https://github.com/relativisticelectron/specter-desktop/tree/notification) into a PR?

I think this looks very promising. Maybe we can somehow combine that with #1695 ? On the browser-side, I'd focus for now on the Javascript Notifications and defer the notification-API to the very end. So imho the order should be:

  • basic notification system (maybe with some pages to read/archive)
  • ZMQ connection integrating with the notification-system
  • Javascript Notifications
  • Notifications API

Thank you so much, you're doing a great work!

k9ert added a commit that referenced this issue Sep 13, 2022
* changed class name

* Working requirements.in

* Reordered lines to see changes better

* Working minimal upgrades

* Added hashes

* - Upgraded python to 3.10 and bitcoin to v23.0
- Added libzmq to build process, to enable future use cases such as #778
- added --without-gui to remove configure warning that gui will not be built.

* Added line-break

* - Updated the embit version to 0.4.13
- pushed python-bitcoind image

* - Build registry.gitlab.com/c8527/specter/cirrus-jammy:20220504
- updated docker references

* - added cypress-base-ubuntu-jammy

* - added cypress-python

* - fixed unchanged registry path

* Fixed wrong path

* readme change

* Fixed path

* Changed github-changelog

* changes registrar links

* Fixed bitcoin version

* Set reset bitcoin version to 22

* Set bitcoin version to 22.0

* Fixed black issue via psf/black#2964 (comment)

* Revert for changelog

* Upgraded pytest because otherwise I get the error  pytest-dev/pytest#9195

* Added temporary fix for upgraded hwi version. Should be fixed in #1693

* more verbosity for page generation

* doc about python 3.10

* docs: update TOC

* yet another library necessary

* black

* Corrected tag

* Fixed wrong python version introduced in the merge commit

* Use same babel version across requirements and test_requirements

* Created the reuqirements.txt with python 3.8 such, that that netlify can create the python docs in python 3.8, while everything elese works also with python 3.10

* testing https://peps.python.org/pep-0508/#environment-markers

* CHanged just the txt

* Added comments

* electron docker build successful

* rename image to cypress-python:v9.7.0

* doc

* docker file rename

* fix pyinstaller pip error by changing requirements.in

* reference jammy electron dockerfile

See electron-userland/electron-builder#6922 (comment)

* updated all references to dockerimages

* fix release-build

* fix the image-version to something else than latest. pin all the stuff!

* revert to old image to avoid glibc issues

Co-authored-by: Kim Neunert <[email protected]>
Co-authored-by: k9ert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants