[RPC] Add RPC long poll notifications #7949

pull jonasschnelli wants to merge 2 commits into bitcoin:master from jonasschnelli:2016/04/rpc_signals changing 8 files +505 −0
  1. jonasschnelli commented at 3:33 pm on April 26, 2016: contributor

    Reasons for another notification interface

    • Currently there is no interface that could be extended to “private” notification secured behind the authorization (like peers connected/disconnected or a new wallet relevant transaction notification)
    • HTTP long poll notifications are very easy to set up and require almost no dependencies
    • HTTP long poll notifications can easily pushed over the internet using httpd reverse proxy together with a popper authentication method (certs or http auth digest) together with TLS.
    • HTTP long poll would allow connecting applications to do all kinds of things with just a single communication channel (currently you need RPC & ZMQ for most use cases which would require VPN or a fancy multi port stunnel connection to broadcast the notification over the internet)

    How does it work

    • The listener calls the pollnotification RPC command.
    • If no notifications are available, the RPC thread will idle for a given timeout (30s by default)
    • If a notification was fired during the 30 seconds, the longpoll call will be responded with the new notification(s)
    • The client/listener can immediately reconnect and wait again
    • If notifications are already in the queue, the pollnotification command will immediately response.
    • Notifications can’t get lost (possible to lose them during http transfer and if one exceed the queue limit)

    Downsides

    • JSON encoding overhead

    New RPC calls

    setregisterednotifications [<notificationtype>] (possible types are hashtx and hashblock) getregisterednotifications pollnotifications

    Missing

    • More tests
    • Documentation

    I’d like to use a such interface to work on a remote GUI (use case: GUI on your local desktop, node on a VPS).

  2. laanwj commented at 3:40 pm on April 26, 2016: member

    I like the concept of being able to listen for events through http, however I think this is severely limited by having server-side state, limiting the number of listeners to only one.

    What I’d personally prefer is, instead of longpolling, to subscribe to a ‘stream’ of events (e.g. websocket or just chunked encoding), where the set of events to listen to is in the request. This avoids having to store any client-state in the server - at least for longer than the request lasts.

  3. jonasschnelli commented at 3:45 pm on April 26, 2016: contributor

    [… ] having server-side state, limiting the number of listeners to only one

    Right. The current implementation limits to only one listener. Extending this PR so it would support a client chosen UUID would not be very complicated (a set of queues and a set of registered notification types). Clients could register notification types along with a client-chosen UUID. I might extend this PR to support multiple listeners.

  4. jonasschnelli added the label RPC/REST/ZMQ on Apr 26, 2016
  5. jonasschnelli commented at 5:45 pm on April 26, 2016: contributor

    Added a commit that allows multiple clients at the same time.

    The new RPC commands require now a clientUUID parameter (a per client unique string, ideally a UUID after RFC 4122). Bitcoind keeps a queue, sequence numbers and registered types per client.

    There is currently not max client limit and no way to remove clients (though you can unregister all notification types but not empty the current queue).

  6. jonasschnelli force-pushed on Apr 26, 2016
  7. jonasschnelli force-pushed on Apr 26, 2016
  8. jonasschnelli force-pushed on May 6, 2016
  9. jonasschnelli commented at 9:53 am on May 6, 2016: contributor
    Rebased. Would be nice to get some concept NACKs/ACKs.
  10. in qa/rpc-tests/rpcsignals.py: in 90b28e41a4 outdated 
    0@@ -0,0 +1,64 @@
1+#!/usr/bin/env python2
    MarcoFalke commented at 10:10 am on May 6, 2016:
    Nit: python3
    jonasschnelli commented at 10:29 am on May 6, 2016:
    Fixed.
  11. jonasschnelli force-pushed on May 6, 2016
  12. jonasschnelli force-pushed on May 12, 2016
  13. jonasschnelli commented at 10:54 am on May 12, 2016: contributor
    Rebased.
  14. jonasschnelli force-pushed on Jul 8, 2016
  15. laanwj commented at 1:09 pm on January 11, 2017: member
    Gah we need to take a look at this again after 0.14 is released.
  16. jonasschnelli commented at 1:27 pm on January 11, 2017: contributor
    Yes. Sure. I’ll try to re-base and overhaul this soon.
  17. laanwj added this to the milestone 0.15.0 on Mar 5, 2017
  18. laanwj added this to the milestone 0.16.0 on Jul 11, 2017
  19. laanwj removed this from the milestone 0.15.0 on Jul 11, 2017
  20. TheBlueMatt commented at 4:47 pm on September 28, 2017: member
    Plan on rebasing this, or should just be closed?
  21. jonasschnelli commented at 4:57 pm on September 28, 2017: contributor
    I’m currently rewriting this… will be ready soon.
  22. jonasschnelli force-pushed on Oct 20, 2017
  23. jonasschnelli force-pushed on Oct 20, 2017
  24. jonasschnelli commented at 6:02 am on October 20, 2017: contributor

    Overhauled and rebased.

    This is still server based (server keeps track of what the client has) queue max size is currently 1024^2 and does only contain hashes of blocks or transactions. Each notification comes with a sequence number to detect lost transactions (which then should trigger a “full-client-sync”).

  25. Add RPC long poll notifications ca6a3f888d
  26. [QA] Add rpcnotification test 2652c55b7a
  27. jonasschnelli force-pushed on Oct 23, 2017
  28. in src/rpc/notifications.cpp:218 in 2652c55b7a 
    213+    }
214+
215+    void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) override
216+    {
217+        LOCK(m_cs_queue_manager);
218+        BOOST_FOREACH (NotificationQueue* queue, m_map_sequence_numbers | boost::adaptors::map_values) {
    TheBlueMatt commented at 5:59 pm on November 6, 2017:
    Ugh, can we not use more BOOST_FOREACH garbage? Should be really easy to rewrite this without, no?
  29. in src/rpc/notifications.cpp:215 in 2652c55b7a 
    210+            // Do a normal notify for each transaction removed in block disconnection
211+            NotifyTransaction(ptx);
212+        }
213+    }
214+
215+    void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) override
    TheBlueMatt commented at 6:00 pm on November 6, 2017:
    I dont think this is the notification we want here - dont we want to use BlockConnected/Disconnected to notify clients of all connected blocks, not just new tips after reorgs which potentially connect multiple blocks?
  30. in src/rpc/notifications.cpp:55 in 2652c55b7a 
    50+
51+class NotificationQueue
52+{
53+public:
54+    std::deque<NotificationEntry> m_queue;
55+    std::map<NotificationType, int32_t> m_map_sequence_numbers;
    TheBlueMatt commented at 6:02 pm on November 6, 2017:
    I generally prefer 64 bit ints here - sure, its unlikely you’d overflow 32 bits, but if you’re online for 3 or 4 years you may start getting closeish.
  31. in src/rpc/notifications.cpp:152 in 2652c55b7a 
    147+        LOCK(m_cs_notification_queue);
148+
149+        size_t queueSize = m_queue.size();
150+        if (queueSize > MAX_QUEUE_SIZE) {
151+            m_queue.pop_front();
152+            LogPrintf("RPC Notification limit has been reached, dropping oldest element\n");
    TheBlueMatt commented at 6:15 pm on November 6, 2017:
    I’d be somwhat worried we’d fill someone’s drive with debug log entries in case they forget to deregister a listener, here.
  32. in src/rpc/notifications.cpp:171 in 2652c55b7a 
    166+
167+class NotificationQueueManager : public CValidationInterface
168+{
169+public:
170+    CCriticalSection m_cs_queue_manager;
171+    std::map<clientUUID_t, NotificationQueue*> m_map_sequence_numbers;
    TheBlueMatt commented at 6:29 pm on November 6, 2017:
    It’d be great if we could de-duplicate the queues here - no need to have a queue per client, just have a global queue and keep track of how far delayed all the clients are in terms of the sequence number and just clean things up to the furthest-back client.
    TheBlueMatt commented at 6:33 pm on November 6, 2017:
    Also, please use unique_ptr here instead of manual management and maybe remove the queue when there are no registered types (and, I suppose, the client is caught up) instead of keeping around a null queue.
  33. in src/rpc/notifications.cpp:352 in 2652c55b7a 
    347+            clientQueue->dequeueElements(result);
348+            break;
349+        }
350+        if (startTime + timeOut + (500 / 1000.0) < GetTime())
351+            break;
352+        MilliSleep(500);
    TheBlueMatt commented at 6:34 pm on November 6, 2017:
    Should probably use a CV and a way for Interrupt to interrupt it instead of calling ShutdownRequested in a 500ms loop.
  34. TheBlueMatt commented at 6:51 pm on November 6, 2017: member
    Concept ACK, though should definitely get more general concept review. I think this could use a more explicit register/deregister process eg registernewclient [ThingsClientCaresAbout] -> provides UUID (instead of registering taking a client-provided UUID), then an explicit deregister which removes queues for that client, instead of setting notifications to 0 and the queues for that client simply leaking.
  35. TheBlueMatt commented at 6:51 pm on November 6, 2017: member
    Also, looks like the test is failing.
  36. NicolasDorier commented at 4:10 pm on January 10, 2018: contributor
    Strong Concept ACK for this one ! @jonasschnelli any idea if you will bring this one from the dead?
  37. jonasschnelli commented at 6:50 pm on January 10, 2018: contributor
    @NicolasDorier I think there is no consensus about an additional push channel… also, @sipa brought up the idea of having a push channel (could be long poll) that acts similar then listsinceblock where the server doesn’t need to keep track of clients (keep a queue). I haven’t looked closer at this approach.
  38. laanwj removed this from the milestone 0.16.0 on Jan 11, 2018
  39. laanwj added this to the milestone 0.17.0 on Jan 11, 2018
  40. NicolasDorier commented at 7:15 pm on January 14, 2018: contributor

    I implemented a similar solution in NBXplorer. Basically there is a GetUTXOs(xPub) call, this call replay all the transactions of the xpub in topological order to create the current utxo for this xpub. While playing the transactions, it hashes them along the way (the hash after each transaction is effectively the equivalent of a bookmark). Then the bookmark + the UTXO is sent back to the client.

    The client process the result, then call again GetUTXOs(xPub, bookmark). The server does the same operation, replaying all transactions while calculating bookmarks along the way, when it reaches the bookmark passed by the client, it knows that what is after is a differential to the previous bookmark. If there is no differential, it just long poll. If there is a differential, it sends it back to the client.

    If the bookmark in parameter was not reached, then the full UTXO is sent back again to the client, with a flag indicating it is not a differential.

    This solution does not involve server side state.

  41. MarcoFalke added the label Needs rebase on Jun 6, 2018
  42. jonasschnelli removed this from the milestone 0.17.0 on Jul 19, 2018
  43. jonasschnelli added this to the milestone 0.18.0 on Jul 19, 2018
  44. in src/rpc/notifications.cpp:28 in 2652c55b7a 
    23+#include <stdint.h>
24+
25+static const char* MSG_HASHBLOCK = "hashblock";
26+static const char* MSG_HASHTX = "hashtx";
27+
28+/* keep the max queue size large becase we don't
    practicalswift commented at 6:37 pm on September 2, 2018:
    Typo found by codespell: becase
  45. in src/rpc/notifications.cpp:86 in 2652c55b7a 
    81+            return NotificationType::Unknown;
82+    }
83+
84+    // populates a json object with all notifications in the queue
85+    // returns a range to allow removing the elements from the queue
86+    // after successfull transmitting
    practicalswift commented at 6:37 pm on September 2, 2018:
    Typo found by codespell: successfull
  46. in src/rpc/notifications.cpp:303 in 2652c55b7a 
    298+            "\"[\"\n"
299+            "\"  \"<signal>\"             (string) The registered signal\n"
300+            "\"  ,...\n"
301+            "\"]\"\n"
302+            "\nExamples:\n"
303+            "\nCreate a transaction\n" +
    promag commented at 3:28 pm on November 8, 2018:
    👀
  47. promag commented at 2:51 pm on November 30, 2018: member
    @jonasschnelli do you plan to pick this again?
  48. laanwj removed this from the milestone 0.18.0 on Feb 6, 2019
  49. fanquake added the label Up for grabs on Jun 17, 2019
  50. fanquake closed this on Jun 17, 2019
  51. laanwj removed the label Needs rebase on Oct 24, 2019
  52. DrahtBot locked this on Dec 16, 2021


jonasschnelli laanwj MarcoFalke TheBlueMatt NicolasDorier practicalswift promag

Labels
RPC/REST/ZMQ Up for grabs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me