Update Chainnotifier Service to leverage batch-request support #1
Conversation
Vib-UX
changed the title
Vib-UX
force-pushed
the
BatchRPC
branch
2 times, most recently
from
8593d86
to
2461109
Compare
chainntnfs/btcdnotify/btcd.go
Outdated
| DisableConnectOnNew: true, | ||
| Certificates: config.Certificates, | ||
| Endpoint: config.Endpoint, | ||
| HTTPPostMode: true, |
There was a problem hiding this comment.
Not sure but I don't think btcd requires http post mode?
There was a problem hiding this comment.
When working with updated batch client HTTPPostMode is required.
| @@ -221,7 +222,7 @@ func testHistoricalConfDetailsNoTxIndex(t *testing.T, rpcpolling bool) { | |||
| unknownConfReq, err := chainntnfs.NewConfRequest(&unknownHash, testScript) | |||
| require.NoError(t, err, "unable to create conf request") | |||
| broadcastHeight := syncNotifierWithMiner(t, notifier, miner) | |||
| _, txStatus, err := notifier.historicalConfDetails( | |||
| txConfirmation, txStatus, err := notifier.historicalConfDetails( | |||
There was a problem hiding this comment.
getting txConfirmation here will increase the coverage by verifying the output results.
|
This is the latest version @Vib-UX ? |
|
It's not the updated one, need to change the replace directive for the updated btcwallet and some nit fix, will push updates by tomm eod. |
Vib-UX
force-pushed
the
BatchRPC
branch
2 times, most recently
from
36dbc33
to
e9cc98f
Compare
| return &chainntnfs.TxConfirmation{ | ||
| Tx: tx, | ||
| BlockHash: blockHashes[start+j], | ||
| BlockHeight: uint32(height - j), |
There was a problem hiding this comment.
I think this is also a + ? Maybe I'm wrong, but since we iterate from currentHeight backwards, height is always the start of the current batch iuuc.
There was a problem hiding this comment.
I think it should be -ve only, as we traverse height from currentHeight to heightHint with height--. Now since we are traversing backwards.
Lets take an example,
currentHeight = 100start=0batchSize = 15- j is traversing from
block=100toblock=86
So here if we found the relevantTxMatch at j=0 we return currentHeight-j --> 100-0 = 100 ✔
| return nil, chainntnfs.TxNotFoundManually, err | ||
| } | ||
|
|
||
| height := int(currentHeight) - start |
There was a problem hiding this comment.
Perhaps worth adding a comment here to explain what we track with height.
| SpenderTxHash: &txHash, | ||
| SpendingTx: tx, | ||
| SpenderInputIndex: inputIdx, | ||
| SpendingHeight: int32(height - j), |
There was a problem hiding this comment.
Again, I believe this should be +, see other comment.
| } | ||
| if !matches { | ||
| continue | ||
| height := int(endHeight) - start |
There was a problem hiding this comment.
Please add some comment there to help readers understand the stepping.
|
Ready to open against upstream once all comments are addressed. 🎉 |
| require.Equal(t, validSpendDetails.SpentOutPoint, outpoint) | ||
| require.Equal(t, validSpendDetails.SpenderTxHash, spendTxHash) | ||
| require.Equal(t, validSpendDetails.SpendingTx, spendTx) | ||
| require.Equal(t, validSpendDetails.SpendingHeight, int32(broadcastHeight+1)) |
There was a problem hiding this comment.
if indexing would be off this would have thrown error.
I compared actualSpendHeight with received BlockHeight from validSpendDetails.
With this RegisterSpendNtfn() & RegisterConfirmationsNtfn() services will leverage the support of updated batchAPI for bitcoind making it lightning fast.
Testcase to ensure the updated historicalSpendDetails() and confDetailsManually() which leverages batchAPI for scanning the chain manually works as expected with enhanced performance.
BatchRPC