Skip to content

Create new signal for notification of new blocks. Use w/ -blocknotify #4599

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.

Sign up for GitHub

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

Jump to bottom
Closed
jgarzik wants to merge 2 commits into from
Closed

Create new signal for notification of new blocks. Use w/ -blocknotify #4599

jgarzik wants to merge 2 commits into from

Conversation

@jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Jul 28, 2014

Drops lock before calling signal. Not purely necessary, but good practice.

@sipa
Copy link
Member

sipa commented Jul 28, 2014

Untested ACK.

@laanwj
Copy link
Member

laanwj commented Jul 29, 2014

Please remove NotifyBlocksChanged as well. There's no point in having both and it's unused.

@jgarzik
Copy link
Contributor Author

jgarzik commented Jul 29, 2014

@laanwj updated

@laanwj
Copy link
Member

laanwj commented Jul 30, 2014

Thanks!

ut ACK.

@jgarzik jgarzik added this to the 0.10.0 milestone Jul 31, 2014
@jgarzik
Copy link
Contributor Author

jgarzik commented Jul 31, 2014

Updated to avoid connecting signal, if -blocknotify absent. Re-tested absent and present cases.

@sipa
Copy link
Member

sipa commented Jul 31, 2014

Nice change to connect the signal conditionally. Two nits:

  • No need to check for the -blocknotify argument being empty.
  • The function being bound can have a much clearer name ("BlockNotifyCallback" ?).

laanwj
laanwj reviewed Jul 31, 2014
View reviewed changes
src/main.cpp Outdated
Copy link
Member

@laanwj laanwj Jul 31, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this callback moved out of main.cpp, as it's a) just a wrapper arond runCommand b) not referred to in main.cpp but only in init.cpp.

Copy link
Contributor Author

@jgarzik jgarzik Aug 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving it out of main.cpp is fine. However, init.cpp should not be home to code continuously executed during program runtime, well after initialization completes.

@jgarzik jgarzik force-pushed the blocknotify-signal branch from c7b6117 to ca1b40d Compare August 25, 2014 18:54
@BitcoinPullTester
Copy link

BitcoinPullTester commented Aug 25, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4599_ca1b40d6ddcfa5d42539fc98b580a3d815a8ef57/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@jgarzik
Copy link
Contributor Author

jgarzik commented Aug 25, 2014

Fixed build bug (#include error).

jgarzik pushed a commit that referenced this pull request Aug 29, 2014
Merge pull request #4599
309aa76
@jgarzik jgarzik closed this Aug 29, 2014
@jgarzik jgarzik deleted the blocknotify-signal branch August 29, 2014 19:29
@jgarzik jgarzik mentioned this pull request Aug 30, 2014
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

0.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jgarzik @sipa @laanwj @BitcoinPullTester