smite-scenarios: sync target chain view via RPC after mining - #211
smite-scenarios: sync target chain view via RPC after mining#211NishantBansal2003 wants to merge 1 commit into
Conversation
Targets previously learned about new blocks through bitcoind's blocknotify, which fires asynchronously. When multiple blocks are mined, this can queue up many sync calls, adding unnecessary load. Instead, explicitly sync the target once after each MineBlocks operation. Introduce a TargetRpc trait with a chain_sync() method, called by the executor immediately after MineBlocks. CLN and LDK sync explicitly via RPC and signals, respectively, while LND and Eclair use ZMQ and remain no-ops. Dropping -blocknotify also removes the burst of asynchronous SIGUSR1s during initial block generation, so LDK no longer needs the pre_exec SIG_IGN. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Good idea! It's much better that it only calls the sync signal once per MineBlock instead of calling it for every block mined.
| assert!( | ||
| out.status.success(), | ||
| "pkill -USR1 ldk-node-wrapper failed: {}", | ||
| String::from_utf8_lossy(&out.stderr) | ||
| ); |
There was a problem hiding this comment.
Should we have an assert here? Imagine the executor crashes the target while executing the input. It would probably fail to sync and panic with a pkill error instead of failing with a Violation::Crashed error.
There was a problem hiding this comment.
Hmm, great point, I think pkill or lightning-cli should always succeed in any case, and they should only error out when the target has already crashed. So, I think we could check whether the target has crashed before asserting this. If it has crashed, we can simply log the failure here, otherwise, we should assert it. I don't think there is any other case where RPC commands should be unresponsive, so I think it's better to be stricter. WDYT?
There was a problem hiding this comment.
Yes, I agree! I also think logging is a good option.
| cmd.env("LD_PRELOAD", handler); | ||
| } | ||
|
|
||
| // Ignore SIGUSR1 for the window between exec and the wrapper blocking |
There was a problem hiding this comment.
Some comments at workloads/ldk/src/main.rs also need to be updated after the remove of SIG_IGN.
2 participants
ref: #195
Targets (CLN, LDK) previously learned about new blocks through bitcoind's
blocknotify, which fires asynchronously. When multiple blocks are mined, this can queue up many sync calls, adding unnecessary load. Instead, explicitly sync the target once after eachMineBlocksoperation.Introduce a
TargetRpctrait with achain_sync()method, called by the executor immediately afterMineBlocks. CLN and LDK sync explicitly via RPC and signals, respectively, while LND and Eclair use ZMQ and remain no-ops.Dropping
-blocknotifyalso removes the burst of asynchronousSIGUSR1s during initial block generation, so LDK no longer needs thepre_execSIG_IGNThe reason I went with an RPC trait is that, in the future, we'll need access to the target's CLI/RPC to add payment hash and preimage information to the target so that it can send an
update_fulfill_htlcmessage.Also, for now, I only added the CLI as a trait implementation for each target to keep the changes contained and concise. In the future, we can definitely add direct RPC calls over the socket as well to save overhead.