Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Drop release times for CNode #2419

Merged
merged 1 commit into from Apr 8, 2013

Conversation

Projects
None yet
5 participants
Owner

sipa commented Mar 28, 2013

It seems there were two mechanisms for assessing whether a CNode was still in use: a refcount and a release timestamp. The latter seems to have been there for a long time, as a safety mechanism.

However, this timer also keeps CNode objects alive for far longer than necessary after disconnects, potentially opening up a DoS window.

This commit removes the timestamp-based mechanism, and replaces it with an assert(nRefCount >= 0), to verify that the refcounting is indeed correctly working.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/307465b04e408e67cca991cf11b8238a487ba01f for binaries and test log.
This is an automated test script which runs test cases on each commit every time is updated.
It, however, dies sometimes and fails to test properly, if you are waiting on a test, please check timestamps and if the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
and contact BlueMatt on freenode if something looks broken.

Owner

sipa commented Mar 31, 2013

Rebased.

Owner

laanwj commented Mar 31, 2013

ACK

BTW maybe we could use RAII or even one of the boost reference counting pointers here, instead of explicit AddRef/Release, as that tends to be somewhat more robust.

@sipa sipa referenced this pull request Apr 2, 2013

Merged

Limited mapAlreadyAskedFor #2423

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/670921ec410923fa70a38dd44fdfec157bc6f8ca 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.

@sipa @sipa sipa + sipa Drop release times for CNode
It seems there were two mechanisms for assessing whether a CNode
was still in use: a refcount and a release timestamp. The latter
seems to have been there for a long time, as a safety mechanism.

However, this timer also keeps CNode objects alive for far longer
than necessary after disconnects, potentially opening up a DoS
window.

This commit removes the timestamp-based mechanism, and replaces
it with an assert(nRefCount >= 0), to verify that the refcounting
is indeed correctly working.
cedaa71
Owner

sipa commented Apr 4, 2013

Rebased.

Member

gmaxwell commented Apr 4, 2013

ACK. This has survived several days of testing and abuse.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/cedaa714462871213472019545b8e862dacdac91 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.

Owner

sipa commented Apr 7, 2013

I've been testing this with an extra commit that prints out vNodesDisconnected.size() at shutdown, and this seems to be always 0. That means that the refcounting is indeed working as intended: it doesn't go too low (tested by the assert), and not too high (as that would mean they linger in vNodesDisconnected until they reach 0).

Contributor

gavinandresen commented Apr 8, 2013

ACK

@sipa sipa added a commit that referenced this pull request Apr 8, 2013

@sipa sipa Merge pull request #2419 from sipa/noreltime
Drop release times for CNode
b7b774d

@sipa sipa merged commit b7b774d into bitcoin:master Apr 8, 2013

@sipa sipa deleted the sipa:noreltime branch May 3, 2013

@laudney laudney pushed a commit to reddcoin-project/reddcoin that referenced this pull request Mar 19, 2014

@sipa sipa Merge pull request #2419 from sipa/noreltime
Drop release times for CNode
ebb4810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment