Blockstream / gdk_rpc

gdk_rpc for bitcoind/liquidd
MIT License
6 stars 7 forks source link

Remove session manager #33

Closed jb55 closed 4 years ago

jb55 commented 4 years ago

I don't see why we need this, but would be happy to hear otherwise. These changes make the API stateless and generally simplify things a bunch.

stevenroose commented 4 years ago

Makes sense. The session manager was there when I started working on this. I think it might have been mimicked from how GDK does things. ACK

shesek commented 4 years ago

I implemented this back then to keep track of the active managed sessions, for two reasons: (1) to avoid doing unsafe dereferencing of unknown session pointers that were not initialized by us (i.e created externally somehow and fed in, without going through GDKRPC_create_session), and (2) in order to issue tick() on every active session from the ticker thread (which I see was removed, I guess because update polling is implemented differently now? I'm still catching up with the recent changes)

jb55 commented 4 years ago

@shesek We're moving away from gdk_rpc as a drop-in C-api replacement, so we don't need a complete re-implementation of the api. It will be more tightly integrated as a new session type within gdk itself. I already moved gdk_rpc into the gdk tree and integrated it directly into the meson build. see https://github.com/jb55/gdk/commits/ga-rpc for progress.

shesek commented 4 years ago

Yes, I'm lurking in the signal chat and read about some of these changes. Will look into your ga-rpc work. Can I develop on this repo for now, or are we freezing changes to gdk-rpc until the migration into gdk is finished?

And how do poll updates work with the new mechanism, is the GDK now responsible for periodically triggering the gdk-rpc "tick"?

jb55 commented 4 years ago

@shesek by ticks do you mean heartbeats for long running connections? trying to understand what that was being used for. if we need to do that, it can be managed from withing session.cpp now.

the reason we decided to migrate it into gdk is that it doesn't really make sense as a standlone library anymore after these changes. you can still contribute to it inside gdk but obviously gdk's build is a lot more complicated. you should be able to pull down that branch and make changes within the subprojects/gdk_rpc folder like normal, and cargo should still work like normal.

the way I do dev right now is to spawn a dev shell (I use nix so the invocation is: nix-shell -p ccache clang pkg-config meson clang-tools ninja libtool automake autoconf swig python3Packages.virtualenv python3 and then I run ./tools/build.sh --clang --buildtype=debug --enable-tests and then ninja -C build-clang -j10 test_rpc. meson should do the cargo build. you can even edit rs files and ninja will re-run the cargo build since I have cargo as a custom target now.

kind of a crazy build but it works. the only issue right now is linker problems on some systems due to conflicts between the rust libsecp256k1 symbols and the ones we build from libwally. I use objcopy on linux to fix this but that doesn't exist on macos, etc.

hopefully this doesn't become too much of an engineering burden otherwise I would be tempted to just build a small rpc module within c++ itself to get full node support out the door and into people's hands.

shesek commented 4 years ago

The "tick" thing was used to poll updates from the rpc server, to check for new blocks and new wallet transactions, and send out updates to the notification handler callback registered via GDKRPC_set_notification_handler (previously GA_set_notification_handler).

Thanks for the development/build instructions, that's very useful!

shesek commented 4 years ago

See tick() in session.rs, which was previously ran periodically (every few seconds) by the ticker thread on all active sessions:

https://github.com/Blockstream/gdk_rpc/blob/4333bb871ddbbfe591d84424c387d0af8f7f1b75/src/session.rs#L40-L47

And updates() in wallet.rs, which tick() delegates to and that does the actual logic of checking for new updates:

https://github.com/Blockstream/gdk_rpc/blob/4333bb871ddbbfe591d84424c387d0af8f7f1b75/src/wallet.rs#L301-L340