Skip to content

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

Closed
jgarzik wants to merge 2 commits intobitcoin:masterfrom
jgarzik:blocknotify-signal
Closed

Create new signal for notification of new blocks. Use w/ -blocknotify#4599
jgarzik wants to merge 2 commits intobitcoin:masterfrom
jgarzik:blocknotify-signal

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