perf: Return the execution tipset in GetMsgInfo

[fridrik01]

Related Issues

#10589

Proposed Changes

This PR updates the GetMsgInfo to include the execution tipset of the message (if it exists). This allows us to call GetTipSetByCid instead of GetTipsetByHeight which should be considerably faster and help speed up both StateSearcMessage and StateWaitForMessage calls.

Additional Info

• I considered having single sql query which returned both the message info and execution tipset (if it exists) but kept it separate for now for simplicity
• Should GetMsgInfo have this feature disable behind a context option?

Test plan

Test functionality
Start lotus node using calibration net

make clean calibnet
lotus daemon --import-snapshot /Users/fridrik/Downloads/calibnet/448080_2023_04_06T08_13_00Z.car --halt-after-import
lotus daemon

Printed out some messages in sqlite/messageindx.db

sqlite3 ~/.lotus/sqlite/msgindex.db
select * from messages order by epoch desc limit 10;
bafy2bzaceb5skhi6jrfyuxzyl6oe52excntshmezxyqkd67anmn63wnxdmwpq|bafy2bzacedm7socnz5gydrnldhgjtbieo7k6kxnk3plqjsrrjgp5dkirmjau2|448547
bafy2bzaceaijcqr7frffx3iaelrws4jap3rfn3bwusz5kv2oqmy22jnsafuji|bafy2bzacecve53tdw3oxlogtsazgb2h46hxynlarqqy57x3s5xadcxueycmvo|448545
bafy2bzacecs2logtqeb55rdryviveymegfyfpw6szhuz7rd6k5o7o5hyzshzo|bafy2bzacecve53tdw3oxlogtsazgb2h46hxynlarqqy57x3s5xadcxueycmvo|448545
bafy2bzacebbts5lo65s5fhuu2sfkajf7wqmh2lgogmbis44vrzkir5rsylyiu|bafy2bzacecy25lehmyilozaboilqxx7cvuaoe4plyzexxbisgtbv4x5a6nq2e|448543
bafy2bzaceccx6vyywh3k7adubdsayy7eh4mezpzz6yjvimglcngtvojn67xcg|bafy2bzacecy25lehmyilozaboilqxx7cvuaoe4plyzexxbisgtbv4x5a6nq2e|448543
bafy2bzacedu2e67ambckjqcewrbdte46f2r5obbpbxwjrswhvl72u4lsl3xj4|bafy2bzacedowm3zjmvimbyddzgozpmuxseq5njncqhaqn7v3m44xlew7bn2ce|448541
bafy2bzaceaxefz7474lvmgnsr26fr3g27knqda6dnbdospx7jxekdhyqn5kms|bafy2bzaceabqamuobnivk4eqf3yre4l2j6e7xuley7c54ryfqulnkjyjsyqi6|448540
bafy2bzacebxq5lldyyas4cftpafiztrup4zi6damircy6k2odisp6xxr4qm2k|bafy2bzacebzzhq2clrsaf2g4xryf37gefn2hfhgvsatcxp2grjanzzy4jplqa|448537
bafy2bzacecauph2ixicmj7ln2et4t3gyzmhkafdnq2zfoa6qi6gaav6brpcx4|bafy2bzacebzzhq2clrsaf2g4xryf37gefn2hfhgvsatcxp2grjanzzy4jplqa|448537

Picked bafy2bzaceaxefz7474lvmgnsr26fr3g27knqda6dnbdospx7jxekdhyqn5kms since it has a entry with epoch+1 and called the RPC endpoint:

> curl -X POST -H "Content-Type: application/json"  --data '{ "jsonrpc": "2.0", "method":"Filecoin.StateWaitMsg", "params": [{"/": "bafy2bzaceaxefz7474lvmgnsr26fr3g27knqda6dnbdospx7jxekdhyqn5kms"}, 12], "id": 0 }' 'http://127.0.0.1:1234/rpc/v0'
{"jsonrpc":"2.0","result":{"Message":{"/":"bafy2bzaceaxefz7474lvmgnsr26fr3g27knqda6dnbdospx7jxekdhyqn5kms"},"Receipt":{"ExitCode":0,"Return":null,"GasUsed":1840943,"EventsRoot":null},"ReturnDec":null,"TipSet":[{"/":"bafy2bzaceamo76fjsnotcbymgxkof37caiaxuur5cehchwq4wlipdl2omctqc"},{"/":"bafy2bzaced6dck4rmesmesxg4eivnbytvrow6erknetblk7jyli3qamybv5aa"}],"Height":448541},"id":0}

Observed (using debug logs not part of this PR) that we found the execution tipset in the message index and called GetTipSetByCid instead of GetTipsetByHeight inside searchForIndexedMsg.

test we are using sqlite index on epoch column

Also confirmed that sqlite uses the newly created index when executing the dbqGetTipsetInfo prepared statement:

> sqlite3 ~/.lotus/sqlite/msgindex.db
EXPLAIN QUERY PLAN SELECT tipset_cid FROM messages WHERE epoch = 448541 LIMIT 1;
QUERY PLAN
`--SEARCH messages USING INDEX tipset_epochs (epoch=?)

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[arajasek]

@fridrik01 This LGTM, but do we know how much faster it is? I'm a bit disinclined to merge it if it isn't a significant improvement. My guess is that it won't be much faster, because if we have a match in this index, then we've likely already walked up to the execution tipset's height, which means the ChainStore's index is likely already warmed up to this height, in which case the perf gain might be negligible.

I don't have any good reason NOT to merge this, though, so feel free to say "just merge it" :P

[arajasek]

Suggested change

	if xtsCid != cid.Undef {
	if xtsCid.Defined() {

[arajasek]

Suggested change

	var xts *types.TipSet = nil
	var xts *types.TipSet

[arajasek]

If I'm not wrong, the MsgInfo isn't actually serialized and stored anywhere, which means we can break the shape of the struct without any migration or other problems. In that case, consider just adding the xTsCid to MsgInfo, instead of making it a separate returned value?

[fridrik01]

@fridrik01 This LGTM, but do we know how much faster it is? I'm a bit disinclined to merge it if it isn't a significant improvement. My guess is that it won't be much faster, because if we have a match in this index, then we've likely already walked up to the execution tipset's height, which means the ChainStore's index is likely already warmed up to this height, in which case the perf gain might be negligible.

I don't have any good reason NOT to merge this, though, so feel free to say "just merge it" :P

@arajasek Probably with the recent improvement in GetTipsetByHeight introduced in #10423 which came before the message epic was created then it may not be worth it to land this as the improvement may not be significant enough.

As I understand the skiplist implementation the worst case cost of GetTipsetByHeight is traversing back 19 tipsets (skiplist is 20, minus 1), which means skipping 19 calls to loadTipSet. That is probably something that is comparable to an extra SQL call. so probably not worth it after all.

Let me know if you agree to that and I'll close this PR

[arajasek]

@fridrik01 I think I agree with you -- let's close this PR for now, we can come back to it if necessary!