Conversation
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Looks pretty good! Added a few ideas for optimizations.
Vib-UX
changed the title
Vib-UX
force-pushed
the
BatchRPC
branch
2 times, most recently
from
8593d86 to
2461109
Compare
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.
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
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
LGTM, but I think the indexing is off. Could be I'm wrong which also means some extra comments could be added :)
🚀
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 ✔
There was a problem hiding this comment.
Perhaps worth adding a comment here to explain what we track with height.
There was a problem hiding this comment.
Again, I believe this should be +, see other comment.
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. 🎉 |
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.
3 participants
BatchRPC