Skip to content

Make Json rpc more efficient #8585

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Make Json rpc more efficient #8585

wants to merge 15 commits into from

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Oct 7, 2025 •
edited
Loading

Unify all the places where we read JSONRPC, using membuf which is the most efficient way, which we didn't always use.

Changelog-None

@rustyrussell rustyrussell added this to the v25.12 milestone Oct 7, 2025
@rustyrussell rustyrussell force-pushed the json-rpc-more-efficient branch 2 times, most recently from 71e9f48 to e5f6efb Compare October 20, 2025 05:36
@cdecker
Copy link
Member

cdecker commented Oct 20, 2025

Very nice PR, and excellent consolidation. I think you may have also addressed one of the major Greenlight issues, with that reparse fix of Blocks. We were spending an inordinate amount of CPU cycles on the JSON parsing and hex decoding, which I think you just side-stepped, with the sanity check of seeing if there is a } in the buffer. It kind of assumes there is no other } than the payload closing tag, but unless we start adding telemetry before the payload, we should be good with this. The alternative would be to keep a running tally of opens vs closes, that could also work.

@cdecker
Copy link
Member

cdecker commented Oct 20, 2025

ACK e5f6efb

@cdecker
Copy link
Member

cdecker commented Oct 20, 2025

There is just one issue: you seem to be reading past the end of the membuf, it looks like an off-by-one error to me ;-)

@rustyrussell
Copy link
Contributor Author

There is just one issue: you seem to be reading past the end of the membuf, it looks like an off-by-one error to me ;-)

Weirdly, this wasn't my bug, just exposing an existing one which I fixed in a different series. I cherry-picked that fix here, too.

@rustyrussell rustyrussell force-pushed the json-rpc-more-efficient branch from e5f6efb to 9d43a5f Compare October 22, 2025 20:28
@rustyrussell
Copy link
Contributor Author

Rebased on libcommon.a now that's merged.

@rustyrussell rustyrussell force-pushed the json-rpc-more-efficient branch from 9d43a5f to 1d89026 Compare October 23, 2025 01:49
@rustyrussell
plugins: fix %*.s typo.
d202e67 
And add a check for new uses creeping in, since it got cut & paste
everywhere.

This means "this is a valid string, but truncate it to this many characters"
vs "%.*s" which means "only read this many characters of string":

```
['lightningd-3 2025-10-23T02:31:40.890Z **BROKEN** plugin-funder: Plugin marked as important, shutting down lightningd!']
--------------------------- Captured stderr teardown ---------------------------
    #0 0x557da58ad1dc in printf_common(void*, char const*, __va_list_tag*) asan_interceptors.cpp.o
    #1 0x557da5aff814 in json_out_addv /home/runner/work/lightning/lightning/ccan/ccan/json_out/json_out.c:239:11
    #2 0x557da59740ce in plugin_logv /home/runner/work/lightning/lightning/plugins/libplugin.c:1777:2
    #3 0x557da5969b6f in plugin_log /home/runner/work/lightning/lightning/plugins/libplugin.c:1934:2
    #4 0x557da595c4f6 in datastore_del_success /home/runner/work/lightning/lightning/plugins/funder.c:161:2
    #5 0x557da598b837 in handle_rpc_reply /home/runner/work/lightning/lightning/plugins/libplugin.c:1072:10
    #6 0x557da598a4b0 in rpc_conn_read_response /home/runner/work/lightning/lightning/plugins/libplugin.c:1361:3
    #7 0x557da5adbea5 in next_plan /home/runner/work/lightning/lightning/ccan/ccan/io/io.c:60:9
    #8 0x557da5ae06ff in do_plan /home/runner/work/lightning/lightning/ccan/ccan/io/io.c:422:8
    #9 0x557da5adfb58 in io_ready /home/runner/work/lightning/lightning/ccan/ccan/io/io.c:439:10
    #10 0x557da5aec2ce in io_loop /home/runner/work/lightning/lightning/ccan/ccan/io/poll.c:455:5
    #11 0x557da59757ac in plugin_main /home/runner/work/lightning/lightning/plugins/libplugin.c:2409:3
    #12 0x557da594fe23 in main /home/runner/work/lightning/lightning/plugins/funder.c:1723:2
    #13 0x7f6572229d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #14 0x7f6572229e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #15 0x557da588b584 in _start (/home/runner/work/lightning/lightning/plugins/funder+0x10d584) (BuildId: 71ba63ab577fc6fa60573d3e8555f6db7d5c584d)

0x624000009d28 is located 0 bytes to the right of 7208-byte region [0x624000008100,0x624000009d28)
allocated by thread T0 here:
    #0 0x557da590e7f6 in __interceptor_realloc (/home/runner/work/lightning/lightning/plugins/funder+0x1907f6) (BuildId: 71ba63ab577fc6fa60573d3e8555f6db7d5c584d)
    #1 0x557da5b2149b in tal_resize_ /home/runner/work/lightning/lightning/ccan/ccan/tal/tal.c:755:13
    #2 0x557da59f2032 in membuf_tal_resize /home/runner/work/lightning/lightning/common/utils.c:203:2
    #3 0x557da5b03934 in membuf_prepare_space_ /home/runner/work/lightning/lightning/ccan/ccan/membuf/membuf.c:45:12
    #4 0x557da59d4289 in jsonrpc_io_read_ /home/runner/work/lightning/lightning/common/jsonrpc_io.c:127:2
    #5 0x557da598a635 in rpc_conn_read_response /home/runner/work/lightning/lightning/plugins/libplugin.c:1366:9
    #6 0x557da5adbea5 in next_plan /home/runner/work/lightning/lightning/ccan/ccan/io/io.c:60:9
    #7 0x557da5ae06ff in do_plan /home/runner/work/lightning/lightning/ccan/ccan/io/io.c:422:8
    #8 0x557da5adfb58 in io_ready /home/runner/work/lightning/lightning/ccan/ccan/io/io.c:439:10
    #9 0x557da5aec2ce in io_loop /home/runner/work/lightning/lightning/ccan/ccan/io/poll.c:455:5
    #10 0x557da59757ac in plugin_main /home/runner/work/lightning/lightning/plugins/libplugin.c:2409:3
    #11 0x557da594fe23 in main /home/runner/work/lightning/lightning/plugins/funder.c:1723:2
    #12 0x7f6572229d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: heap-buffer-overflow asan_interceptors.cpp.o in printf_common(void*, char const*, __va_list_tag*)
Shadow bytes around the buggy address:
  0x0c487fff9350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c487fff9360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c487fff9370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c487fff9380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c487fff9390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c487fff93a0: 00 00 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa
  0x0c487fff93b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c487fff93c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c487fff93d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c487fff93e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c487fff93f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==26122==ABORTING
```

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
plugins: make fatal errors neater.
7de6259 
Without this they get run together on stderr.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common: export helper membuf_tal_realloc.
8f4d75d 
We have to call it membuf_tal_resize() because the other on is
a ccan/json_out static function!

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common: add json_dup_contents() to duplicate toks and buffer.
1e16457 
We do this in several places, might as well make it common code.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
plugins: remove unused json_buffer / json_toks members from libplugin…
032579e 
…-pay.

They were never referenced, and saving the toks is questionable since their
lifetime is not guaranteed to live beyond this call (at least the buffer was
duplicated, but that also assumed it was at the start of the object).

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common/jsonrpc_io: helper routines for reading JSON from sockets.
caa8f32 
The efficient way to do this is to use membuf, which handles the buffer
control (only using memmove when necessary).  We have multiple places
where we opencoded this, some of which did not use membuf at all.

So now we create common infrastructure.  I tried making it a single
function but the various users are quite different, so instead I opted
for a toolbox approach.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
libplugin: use jsonrpc_io for reading replies to our async commands.
fd4d658 
This will also be more efficient than doing memmove every time.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
libplugin: wean ld_command_handle off referncing plugin->buffer.
2984d50 
Hand it in as a parameter to reduce churn in next patch.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
libplugin: use jsonrpc_io for stdin from lightningd.
0970c4f 
This is also more efficient if there are many commands at once.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: wean parse_request off referencing jcon->buffer.
bc02585 
Hand it in as a parameter to reduce churn in the next patch.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: use jsonrpc_io for reading JSON commands.
bec2394 
This is more efficient if we have lots of incoming commands, too.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common: add brace hack for jsonrpc_async_parse.
8b5fefb 
This is a trick from bcli: we ask bitcoind for the block, and it hands
us a 2MB hex blob (which we read in multiple parts).  Our parser wades
through it all, but a quick search for '}' makes it much faster.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: wean pligun_log_handle/plugin_notify_handle/plugin_respon…
14461e5 
…se_handle off plugin->buffer.

Hand buffer in as a parameter to reduce churn in the next patch.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: use jsonrpc_io for plugin JSON commands.
0b8658f 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
libplugin: use jsonrpc_io logic for sync requests too.
e1e9f12 
It's a little overkill, but it's clear.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell rustyrussell force-pushed the json-rpc-more-efficient branch from 1d89026 to e1e9f12 Compare October 23, 2025 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v25.12

Development

Successfully merging this pull request may close these issues.

2 participants

@rustyrussell @cdecker