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

enable fast resume of previous torrents #27 #126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bsuh
Copy link
Contributor

@bsuh bsuh commented Jul 5, 2015

Write the bitfield of downloaded pieces to disk and the hash of the
bitfield as well to verify the file isn't corrupted

Write the bitfield of downloaded pieces to disk and the hash of the
bitfield as well to verify the file isn't corrupted
@mafintosh
Copy link
Owner

What if the actual data files gets corrupted? This wont catch that right?
On Sun 5 Jul 2015 at 12:42 Brian Suh notifications@github.com wrote:

Write the bitfield of downloaded pieces to disk and the hash of the

bitfield as well to verify the file isn't corrupted

You can view, comment on, or merge this pull request online at:

#126
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#126.

@bsuh
Copy link
Contributor Author

bsuh commented Jul 5, 2015

No it won't. From what I could tell (from documentation not source code FWIW), libtorrent-rasterbar doesn't check for data corruption either. qBittorent does seem to keep store the last modified date of files and recheck hashes if there's a discrepancy.

AFAIK there's no way to check for data corruption without actually hashing the files, which is what I'm trying to avoid in the first place.

@bsuh
Copy link
Contributor Author

bsuh commented Jul 5, 2015

What could be done is to send 'have' messages to peers based on the fast resume data, but check a piece hash when a peer requests it or it is being read by a file stream, if there's a discrepancy, then choke the peer and refetch the piece.

@asapach
Copy link
Collaborator

asapach commented Jul 5, 2015

You don't want to check the hash every time you access a piece, because it will kill performance. This means you'll have to maintain some kind of bitfield of verified pieces, which increases complexity.

@bsuh
Copy link
Contributor Author

bsuh commented Jul 5, 2015

Right, but only if we want to make sure there's no filesystem corruption. My opinion is that if a user's filesystem is trashing files, then it's the user's problem.

@asapach
Copy link
Collaborator

asapach commented Jul 5, 2015

It's the swarm's problem if you start distributing corrupt pieces.

@bsuh
Copy link
Contributor Author

bsuh commented Jul 5, 2015

Don't clients just block you?

@asapach
Copy link
Collaborator

asapach commented Jul 5, 2015

They should, but you never know. https://en.wikipedia.org/wiki/Torrent_poisoning

@bsuh
Copy link
Contributor Author

bsuh commented Jul 6, 2015

So is there something I can do to get this merged?

@mafintosh
Copy link
Owner

@bsuh could we make this opt-in for now? then after we test it for a bit in the wild we can make it default behaiviour

@bsuh
Copy link
Contributor Author

bsuh commented Jul 6, 2015

Yea, I'll make it a parameter for opts tomorrow. I'm feeling hyperactive and it's hard to focus today.

@mafintosh
Copy link
Owner

we all know that feeling :)

@bsuh
Copy link
Contributor Author

bsuh commented Jul 8, 2015

Added it as an option.

Hmm. When fast resume is enabled, this seems to break the blocklist test, because 'blocked-peer' gets emitted before 'ready' event. The test attaches the 'blocked-peer' only after the engine's 'ready' event.

I don't know if the expectation of 'ready' event always coming before 'blocked-peer' event is a valid expectation. If that's how it's supposed to work, it won't always be true, since engine initialization is asynchronous. It seems that with a fresh torrent with no written data and just 1 file, the one fs.exists call during verification will be fast enough that the 'ready' event will be emitted before 'blocked-peer' event, but one fs.readFile for the non-existent fast resume data file isn't fast enough. Also if previously written data exists, then you need multiple fs calls to verify. So I think the test should be changed?

test('peer should block the seed via blocklist', function(t) {
    t.plan(3);
    var peer = torrents(torrent, {
        dht: false,
        blocklist: [
            { start: '127.0.0.1', end: '127.0.0.1' }
        ]
    });
    peer.once('ready', function() {
        t.ok(true, 'peer should be ready');
        peer.once('blocked-peer', function(addr) {
            t.equal(addr, '127.0.0.1:6882');
            peer.destroy(t.ok.bind(t, true, 'peer should be destroyed'));
        });
    });
});

to

test('peer should block the seed via blocklist', function(t) {
    t.plan(3);
    var peer = torrents(torrent, {
        dht: false,
        blocklist: [
            { start: '127.0.0.1', end: '127.0.0.1' }
        ]
    });
    peer.once('ready', function() {
        t.ok(true, 'peer should be ready');
    });
    peer.once('blocked-peer', function(addr) {
        t.equal(addr, '127.0.0.1:6882');
        peer.destroy(t.ok.bind(t, true, 'peer should be destroyed'));
    });
});

@muchweb
Copy link

muchweb commented Feb 25, 2016

I have stopped a download halfway through and then re-started a program. It seems that it just starts the whole download all over again.

Please implement a fast check and resume feature the way other clients do it.

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.

None yet

4 participants