Skip to content

test_lightningd.py: lightningd currently calls getblock, not getblockhash. #1116

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

Closed
wants to merge 1 commit into from

Conversation

bitonic-cjp
Copy link
Contributor

Without this change, the test fails for me. To be honest, I don't know how this could ever have worked, or what change made it fail.

For me, this test (without the patch) already failed in the commit that added it (737a714), so it doesn't seem to be a later regression.

@cdecker
Copy link
Member

cdecker commented Feb 27, 2018

Just encountered the same issue. getblockhash is usually called to learn about the next height's hash and then getblock is used to retrieve that specific block. However it seems that the test framework doesn't catch the getblockhash call (especially when running with valgrind in my case), so we loop on getblock forever.

@bitonic-cjp
Copy link
Contributor Author

The Travis bot seems to disagree: in its log, I see getblockhash (and strange enough no getblock?). What does this depend on?

@ZmnSCPxj
Copy link
Contributor

I believe this to be a timing issue. On some systems the startup is long enough that the daemon has switched to getblock, while on the faster CI the daemon is still doing getblockhash.

@bitonic-cjp
Copy link
Contributor Author

In that case, what would be a robust fix? We could get rid of both - it seems good enough to just check for estimatesmartfee. Or we could make a more permissive regex that accepts both.

@ZmnSCPxj
Copy link
Contributor

I think it best to summon @rustyrussell for an opinion, as it is a test for code he added.

@rustyrussell
Copy link
Contributor

Sorry, I have fix for this pending in my queue, since I also hit it locally.

I'll push in a separate PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants