Skip to content
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

Never follow bitcoind backwards #3274

Conversation

@rustyrussell
Copy link
Contributor

rustyrussell commented Nov 19, 2019

This solves the underlying reason why #3243 was such a problem!

@rustyrussell rustyrussell requested a review from cdecker as a code owner Nov 19, 2019
sync_blockheight(bitcoind, [l1])
l1.stop()

# Now shrink chain (invalidateblock leaves 'headers' field)

This comment has been minimized.

Copy link
@cdecker

cdecker Nov 19, 2019

Member

Just tried it myself, and indeed the headers remains unchanged after invalidateblock, however if invalidateblock is followed by a restart headers will reset to the expected value. Maybe that's a more stable way of going backwards than blowing away the underlying directory?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Nov 19, 2019

Author Contributor

Ah, that would have been easier!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/bitcoind-never-backwards branch from e09d68d to 528b975 Nov 20, 2019
@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Nov 20, 2019

Rebased to use the Decker technique for creating reorgs...

time.sleep(5)
assert l1.rpc.getinfo()['blockheight'] == 110
Comment on lines +1190 to +1199

This comment has been minimized.

Copy link
@cdecker

cdecker Nov 20, 2019

Member

This could be made into a wait_for. Happy to fix it in a cleanup though.

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Nov 21, 2019

Author Contributor

No, it can't: we want to make sure it's still at 110.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Nov 20, 2019

Seems we have a bit of collateral damage:

  • test_db_dangling_peer_fix uses a DB that was synced up to 205 while we migrate on a chain that has 101 blocks, need to generate 104 more blocks before starting the node, or specify a rescan.
  • test_rescan seems to believe it was once synced to a 500k chain, and is being started with a 101 block chain.

Other than that LGTM 👍

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/bitcoind-never-backwards branch from 528b975 to 7cbfbc4 Nov 21, 2019
@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Nov 21, 2019

Seems we have a bit of collateral damage:

* `test_db_dangling_peer_fix` uses a DB that was synced up to 205 while we migrate on a chain that has 101 blocks, need to generate 104 more blocks before starting the node, or specify a rescan.

* `test_rescan` seems to believe it was once synced to a 500k chain, and is being started with a 101 block chain.

Other than that LGTM +1

Thanks, fixed.

I actually like the change in behavior (refusing to start) if you ask it to rescan from a not-yet-present block, so I've kept and tested that.

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/bitcoind-never-backwards branch from 7cbfbc4 to 10a74c7 Nov 21, 2019
This leads to all sorts of problems; in particular it's incredibly
slow (days, weeks!)  if bitcoind is a long way back.  This also changes
the behaviour of a rescan argument referring to a future block: we will
also refuse to start in that case, which I think is the correct behavior.

We already ignore bitcoind if it goes backwards while we're running.

Also cover a false positive memleak.

Changelog-Fixed: If bitcoind goes backwards (e.g. reindex) refuse to start (unless forced with --rescan).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/bitcoind-never-backwards branch from 10a74c7 to cdafcf3 Nov 21, 2019
@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Nov 21, 2019

Ack cdafcf3

Fixed all the test collateral damage.

@rustyrussell rustyrussell merged commit 654faa6 into ElementsProject:master Nov 21, 2019
4 checks passed
4 checks passed
bitcoin-bot/acks Acks by rustyrussell
bitcoin-bot/changelog This PR has at least one changelog entry
bitcoin-bot/fixups PR does not contain unsquashed fixups
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.