Remove tx from AlreadyAskedFor list once we receive it, not when we process it. #4460

I see no reason not to erase a requested tx this from the AlreadyAskedFor list at the point the tx is received. Otherwise the risk is that the node continues requesting it from peers. (It will of course get re-added if its inv is seen and it's not processed, but this is already the case also).

Uh. If you remove it from the AlreadyAskedFor map and we rejected it, we'll potentially end up requesting it again from the same peer. Or am I missing something there?

There is actually nothing in place to prevent re-asking for an invalid tx every time an inv is received for it; that's actually the cause of ongoing spurious DoS bans. I have proposed #4542 to address this.

Ooh.. Nice work with #4542 - have been waiting for something like this for a while. @gmaxwell Bear in mind that "AlreadyAskedFor" is a misleading name. Read it as "WaitingFor" instead.

PS. rebased

@rebroad mapAlreadyAskedFor is what prevents asking additional peers for the same tx before it meets the AlreadyHave criteria. AlreadyHave doesn't currently return true if the tx is not an orphan but is rejected from the mempool; so as it stands, rejected tx must not be removed from the map since they are never included in AlreadyHave. #4542 would make this PR work.

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

@rebroad This makes a lot of sense. Currently, entries of mapAlreadyAskedFor linger there forever if the transaction is invalid. Removing them when the requested data arrives is the correct thing to do.

The task of mapAlreadyAskedFor was never to make sure that we don't request anything again. It just keeps track of a timestamp for retries. Indeed, the name is misleading.

(so this is part of the functionality of requesting a piece of data; the decision of whether to request something in the first place needs to be made somewhere else, #4542 makes sense there)

Tested ACK.

@laanwj #1341 was raised a while back due to the misleading name of this, and the potential bugs that could be introduced due to thinking it's function was something other than what it is...

I've merged this as a temporary fix until #4831 can be merged, which gets rid of that data structure entirely.