waddrmgr: Put client request first in recovery.#979
Conversation
|
Oh wait, got stuck a second time at another block |
JoeGruffins
force-pushed
the
dontfetchbeforebday
branch
from
40d5f25 to
6935a12
Compare
|
If there was an error connecting to a client inside an update in recovery, the tx is rolled back but the in memory sync point is not. this fixes that https://github.com/btcsuite/btcwallet/compare/40d5f25cd330609e350818655632b18fb1a360c8..6935a129d7e68f106895c7dd0c761d8e09b1fdd9 |
JoeGruffins
force-pushed
the
dontfetchbeforebday
branch
from
6935a12 to
469713a
Compare
JoeGruffins
changed the title
|
The birthday thing was apparently a coincidence. |
wallet/wallet.go
Outdated
| ) | ||
| }) | ||
| if err != nil { | ||
| w.Manager.SetSyncedToMemory(&rollbackStamp) |
There was a problem hiding this comment.
Hmm, so what was the initial error in the first place that got this out of sync? Sounds like all the failed to fetch block hash for height 870852: block not found errors are follow-up errors.
Would be nice to know that. But this fix definitely makes sense.
There was a problem hiding this comment.
I have the causing error in the issue for one instance.
There was a problem hiding this comment.
Ah, I see, fetching a block failed in a Neutrino setup.
Great catch, I'm quite certain this is indeed the cause for the errors you saw.
I wonder how we should deal with all the other cases where SetSyncedTo() is called within a database transaction that might be rolled back and cause the DB and in-memory state to de-synced...
There was a problem hiding this comment.
I think the out-of-synced state will make things worse. It looks like recoverScopedAddresses should be performed first before calling SetSyncedTo. Also it needs some refactor to move the non-db operations out so we don't need to wrap them in one giant tx walletdb.Update.
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
I don't think this approach works - it's hacky and creates inconsistent states.
wallet/wallet.go
Outdated
| ) | ||
| }) | ||
| if err != nil { | ||
| w.Manager.SetSyncedToMemory(&rollbackStamp) |
There was a problem hiding this comment.
I think the out-of-synced state will make things worse. It looks like recoverScopedAddresses should be performed first before calling SetSyncedTo. Also it needs some refactor to move the non-db operations out so we don't need to wrap them in one giant tx walletdb.Update.
I'll look again but I think all of the db functions never happen, and this was the only thing not rolled back. I'll do whatever though because this bug is affecting a few of our projects. |
|
@yyforyongyu I've looked through, and there is alot, but I can't find any values that are not rolled back. Could you point them out? Also, with no fix, if the bug is hit, then the db would be borked anyway if what you say is correct. Even if a huge refactor is needed, a fix for the moment would be good to have. I can try to write a test I guess? |
JoeGruffins
force-pushed
the
dontfetchbeforebday
branch
from
469713a to
34a13fb
Compare
JoeGruffins
changed the title
|
@yyforyongyu Is this current version ok for a patch? It only swaps the order as you suggested. Because the client request for headers happens in the first part, and this is where a connection error can happen, it won't continue to update the internal sync point. This also fixes the immediate problem for us. https://github.com/btcsuite/btcwallet/compare/469713a564568f734e51e8bd87e7a2ac2885b754..06463724d4c69dc3e09f2191de442f5635e9c606 |
JoeGruffins
force-pushed
the
dontfetchbeforebday
branch
from
34a13fb to
0646372
Compare
JoeGruffins
force-pushed
the
dontfetchbeforebday
branch
from
0646372 to
7509f6b
Compare
|
Combined the if statement with what was above https://github.com/btcsuite/btcwallet/compare/06463724d4c69dc3e09f2191de442f5635e9c606..7509f6b4b9e3aba479cd0f75b171f3607aaf4132 |
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Thanks for the update! LGTM 🙏
JoeGruffins
force-pushed
the
dontfetchbeforebday
branch
from
7509f6b to
fe45f81
Compare
|
I think this change should be safe, thanks a lot for the fix. |
Attempting to do this but having a hell of a time with module tags. Erros like So tried just setting a new tag on my master with that commit and then its Maybe it will be found after a while... |
|
most tests passing locally JoeGruffins/lnd#1 test results |
|
You just add |
THX! that was easy.. its updated now. same tests are passing |
|
@JoeGruffins Thanks for putting up the tests! Could you open a PR in |
|
Confirmed the CI passed in |
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Thanks for the fix, LGTM 🎉
|
@yyforyongyu I think we might need to roll this change back and approach it in a different way. |
4 participants
closes #978
I'm unsure if this is the best solution but it gets me past the block in my issue. It looks to me like this should always hit when restoring with a birthday, but it does not. Does btcwallet not download all headers anyway?