allow breaking inside for_blocks closures using ControlFlow

[antonilol]

bitcoind will keep sending blocks even if ControlFlow::Break(_) if returned, so they have to be received to not interfere with future calls. blocks technically do not have to be parsed then, which happens now at the sender side of blocks_recv, but this makes more sense to optimize that together with using bitcoin_slices (#913 moves parsing of the block to the receiver side).

is it possible to send bitcoind some message to stop sending blocks? otherwise it would be possible to request a few at a time and stop when ControlFlow::Break(_) is returned to prevent having to receive (and bitcoind having to read from disk and send) many blocks.

i also rewrote lookup_transaction to use this method of breaking, which will save at least parsing unneeded blocks later (#913). the only change to this function right now is for_blocks will keep the state with the ControlFlow enum instead of lookup_transaction with Option

i also happened to need this when making a poc implementation for is_unused (#920)

[romanz]

Thanks for the contribution!
Will review after #913 is merged.

[romanz]

IIUC, this optimization is used for optimizing txid index scan (by early-exiting the iteration).
Since currently txid index shouldn't have collisions (except BIP-30 transactions), I'd prefer to keep the existing code for simplicity.
I have opened #928 to make sure there are no more collisions.

[antonilol]

IIUC, this optimization is used for optimizing txid index scan (by early-exiting the iteration)

yes, exiting early means that after the right one if found no more block parsing and hashing has to be done. actually, i just found out that #913 changed this function to return the last matching transaction, whereas before it returned the first one. i think it is best to get the last one as according to bip 30 it is allowed to have duplicate txids, just not duplicate utxos, and i think that it is more likely that a transaction is found in more recent blocks, so reversing blockhashes before it is passed to for_blocks will fix this and optimize this even more, and make the algorithm consistent with what it is now (current master, after #913)

Since currently txid index shouldn't have collisions (except BIP-30 transactions)

the may be collisions in txids as long as no utxos from the utxo set would be overwritten by it.

I'd prefer to keep the existing code for simplicity.

i dont see a complexity increase in this code, and find this well worth the optimization

[antonilol]

reversing blockhashes is somewhat harder that sticking a .rev() adapter in between, but with rocksdb::IteratorMode a backwards iterator can be made, out of scope for this pr, but surely an interesting optimization

(btw that force push was just a rebase)

[romanz]

the may be collisions in txids as long as no utxos from the utxo set would be overwritten by it.

Not sure - do you mean that two different transactions having the same txid?
I think this is extremely unlikely (assuming that SHA-256 is collision resistant).

[antonilol]

the may be collisions in txids as long as no utxos from the utxo set would be overwritten by it.

Not sure - do you mean that two different transactions having the same txid? I think this is extremely unlikely (assuming that SHA-256 is collision resistant).

i meant that two transaction having the same txid is allowed in bitcoin, as stated in bip 30. if this would happen i believe it would be more likely because of a bug than a collision of sha 256

if a user requests a transaction by txid, which transaction should be returned? before #913 this was the first one, now every transaction it finds is assigned to a variable, overwriting a previous result, resulting in the last one being returned

[romanz]

I think that #933 should resolve the ordering (returning the first matching tx), as well as not parsing the next blocks.

[antonilol]

I think that #933 should resolve the ordering (returning the first matching tx), as well as not parsing the next blocks.

i think as well (it reuses the old logic). actually, such a thing should probably be clarified in the electrum server protocol spec.

that fixes one point of this pr (i actually found out about it later than making this). i would like to have the possible optimization i mentioned in the first message, to save bitcoin core from having to send all blocks after the tx has already been found, using this method of 'telling' for_blocks the at first requested blocks are no longer needed. i will try to do some testing on how much time/resources this saves and implement it in the coming week