Light client logs should include 'from_block' when querying logs

[cheme]

Fixes #8560 .

When using a websocket on logs, there was no entry displayed. The issue was that we called light client get logs method for each block (with a range of one block in the filter), but the method did not include the first block to query (with a range of one block it did include nothing).

This PR fixes this by making LightClient log works correctly (by including the start block of the filter).

For the websocket usecase it also increases OnDemand query a lot, so this other PR may be helpfull : #9318 (during my testing without #9318 the lightclient became a bit irresponsive after a while). There is also probably room for optimization in function 'notify_log', yet it should not directly impact network query (if OnDemand cache works as intended of course).

I did try to change as less as possible code, but from a personal point of view I would rather use :

      let mut receipts_futures: Vec<_> = Vec::new();
      for hdr in self.client.ancestry_iter(filter.to_block) {
        let end_cond =  BlockId::Hash(hdr.hash()) == filter.from_block
          || BlockId::Number(hdr.number()) == filter.from_block;

        let hdr_bloom = hdr.log_bloom();
        if bit_combos.iter().find(|&bloom| hdr_bloom & *bloom == *bloom).is_some() {
          let hdr_number = hdr.number();
          receipts_futures.push(
            self.on_demand.request(ctx, request::BlockReceipts(hdr.into())).expect(NO_INVALID_BACK_REFS)
            .map(move |x|{ (hdr_number, x) })
            );
        }
        if end_cond { break; }
      }
      let receipts_futures = receipts_futures;

than

			let receipts_futures: Vec<_> = self.client.ancestry_iter(filter.to_block)
				.take_while(|ref hdr|{
					BlockId::Hash(hdr.hash()) != filter.from_block
					&& BlockId::Number(hdr.number()) != filter.from_block
				})
				.chain(::std::iter::once(self.client.block_header(filter.from_block).expect("checked before")))
				.filter(|ref hdr| {
					let hdr_bloom = hdr.log_bloom();
					bit_combos.iter().find(|&bloom| hdr_bloom & *bloom == *bloom).is_some()
				})
				.map(|hdr| (hdr.number(), request::BlockReceipts(hdr.into())))
				.map(|(num, req)| self.on_demand.request(ctx, req).expect(NO_INVALID_BACK_REFS).map(move |x| (num, x)))
				.collect();

So if the reviewers prefer the more imperative version please tell me.

[parity-cla-bot]

It looks like @cheme signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[debris]

this is a bug. this take_while will never return false therefore this iterator will never end

[ordian]

take_while(a).take_while(b) is equivalent to take_while(a && b), right?
Should we check for the BlockId::Latest case as well?

[cheme]

Seems correct to me (also when testing). I can still switch back to using two take_while (does not make a lot of difference as it combines correctly).

[debris]

not related to this pr, but it's more optimal to use contains_bloom

[cheme]

Yes, this kind of logic should definitely use dedicated function.

[debris]

I think, you can just write

.chain(self.client.block_header(filter.from_block).ok())

Option implements iterator

[ordian]

take_while(a).take_while(b) is equivalent to take_while(a && b), right?
Should we check for the BlockId::Latest case as well?

[ordian]

block_header can return None and I don't see where do we check it's not None, could you point me to that line?

[cheme]

It was tested in the case of a block hash (in block_number ) and I added a condition before to make sure the number case would be in range to, it is certainly not explicit enough.
I did a little change to test it explicitly in my last commit (plus remove the need to have two condition in takewhile).

[andresilva]

LGTM

[andresilva]

Minor nit but I would put the => in the previous line, I don't know what's our style on this though.