New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
multi: source index notifs. from block notifs. #2256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to wait until the prequisite PR is finalized and this is rebased and upgraded accordingly before final approval, but I did want to comment that the overall approach seems fine here.
30884d3
to
51f68b3
Compare
This will need a rebase after #2219 is merged and is also failing the build. |
2f0cd39
to
489f467
Compare
489f467
to
0e3d934
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to test this before approving, but the updated version looks good. I don't see any issues code wise and it is much cleaner and more efficient this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working as intended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me other than I noticed that IsTreasuryEnabled
is not being set correctly on indexers.IndexNtfn
. I noticed this is also the case here in this code that is already merged:
dcrd/blockchain/indexers/common.go
Lines 535 to 541 in ed51788
ntfn := &IndexNtfn{ | |
NtfnType: DisconnectNtfn, | |
Block: block, | |
Parent: parent, | |
PrevScripts: prevScripts, | |
Done: make(chan bool), | |
} |
0e3d934
to
52470c7
Compare
I'll be testing this with the latest updates tonight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I tested this by dropping all of the indexes and allowing them to be reindexed from scratch and then performing the various RPCs that make use of said indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Are you planning on also updating this location in a separate PR? Just want to make sure we don't forget about that.
Whoops, good catch @rstaudt2. Missed that, resolving. |
52470c7
to
fad1bc0
Compare
This updates the index subscriber to use block notifications as the source of index notifications.
fad1bc0
to
8b78cc2
Compare
Depends on #2219
This updates the index subscriber to use block notifications as the source of index notifications.