get_list_of_payments() does not search through the right range of blocks

[PaweuB]

EDIT: Description rewritten by @cameel

Currently get_list_of_payments() does not use the correct range of block when searching for payments.

The range of blocks should be as follows:

1. When searching for regular payments or settlement payments:
   • start block: first block with timestamp >= payment_ts. Then filter out payments for which closure_time < payment_ts.
   • end block: last block - SCIImplementation.REQUIRED_CONFS
     • If there are less than SCIImplementation.REQUIRED_CONFS block overall, then no block qualify
2. When searching for forced subtask payments or additional verification payments:
   • start block: a few blocks before first block with timestamp >= payment_ts. How much exactly is "a few" should be determined by a constant. 10 should be enough in nearly all situations.
   • end block: last block - SCIImplementation.REQUIRED_CONFS

Actually, after recent talks with Łukasz on Rocket, turns out that 2. is not necessary at all. We'll never need to search for forced subtask payments or additional verification payments and we need to revert #982. The rules are given just in case it's needed after all but we should really just to remove support for searching for forced subtask payments.

[dybi]

@PaweuB I know - just do it

[PaweuB]

Pivotal:
https://www.pivotaltracker.com/story/show/162197293

[cameel]

Based on #996 blueprint I think this needs to work differently for regular payments (batch transfers) and for settlement payments.

When you are looking for regular payments you want:

• start block: first block with timestamp >= payment_ts. Then manually filter out payments for which closure_time < payment_ts.
• end block: last block - SCIImplementation.REQUIRED_CONFS
  • If there are less than SCIImplementation.REQUIRED_CONFS block overall, then no block qualify

For forced subtask payments you want start block to be the above minus some safety margin (let's say 10 blocks). Then you go through those extra 10 blocks and see if there are any extra forced subtask payments matching any of the subtask ID submitted by the provider. You take those but ignore all those with IDs that do not match (this is specified #1005 so you should really wait for that one before fixing this).

The safety margin ensures that if the forced subtask payments happens to match a subtask with payment_ts that's slightly in the future, Concent can still find it. See the blueprint for details.

[cameel]

I have updated the description with stuff from my comment above. I've also added the information that this function does not really need to return forced subtask payments.

Below the old description in case someone is confused by the comments here:

@dybi @cameel

it needs description

EDIT:
BlocksHelper(payment_interface).get_first_block_after(payment_ts).number raises exception when payment_ts is after publication of the newest block

[dybi]

@cameel, how does this relate to current implementation of settle_overdue_acceptances(), where:

        # Concent gets list of forced payments from payment API where T0 <= payment_ts + PAYMENT_DUE_TIME.
        list_of_forced_payments = service.get_list_of_payments(  # pylint: disable=no-value-for-parameter
            requestor_eth_address=requestor_ethereum_address,
            provider_eth_address=provider_ethereum_address,
            payment_ts=oldest_payments_ts,
            transaction_type=TransactionType.FORCE,
        )

?

In other words - we don't need it anymore? Or shall it be created differently?

[cameel]

• get_list_of_payments() currently does not take REQUIRED_CONFS into account. It should.
• get_list_of_payments() currently does not filter out payments with closure_time that's too old. But I think settle_overdue_acceptances() does it afterwards so it could be left as is.
  • This filter could be moved into get_list_of_payments() though because we don't really need the low level operation of fetching payments from between two block timestamps. Concent can safely operate only on closure_time timestamps. But it's just internal code organization. The decision here is up to you. This issue is talking about get_list_of_payments() because that's what the original question was about but the important thing here is simply making sure that we're taking into account the right payments, not necessarily which part of the code does this.
  • By the way, as stated in Confusing names in sci_backend #1018, using payment_ts as the parameter name here is misleading. It does not tell you whether the function expects a block timestamp or closure_time. I think you should change that one when you change this function.

[dybi]

@cameel,

1. When searching for regular payments or settlement payments:
   start block: first block with timestamp >= payment_ts. Then filter out payments for which closure_time < payment_ts.

quite frankly, it should be:

first block with timestamp > payment_ts

1. If payment is done in moment X, I assume it is not possible to have it in block which was published in the very same moment X
2. the helper function from golem_sci is get_first_block_after

EDIT:

If there are less than SCIImplementation.REQUIRED_CONFS block overall, then no block qualify

then what? raise exception?

[cameel]

@dybi

quite frankly, it should be:

first block with timestamp > payment_ts

1. If payment is done in moment X, I assume it is not possible to have it in block which was published in the very same moment X

No, it should be block timestamp >= payment_ts.

Block timestamp is not the time when block was published. It's usually close to the time of the publication of the previous block. But it can be inaccurate or even manipulated by the miner so we don't want to assume this. We really only care about closure_time and the only assumption about block timestamps we can make is that closure_time <= block timestamp because the contract validates that.

I know the relations between all those timestamps are confusing and I spent a lot of effort first trying to understand it myself and then to explain it in blueprint #996. Please read the section titled "Blockchain and the timeline", and especially "Accuracy of block timestamps". There are even several examples after that which show timestamps on the blockchain in addition to timestamps in messages. I hope I explained it well but If anything is still unclear, please ask.

the helper function from golem_sci is get_first_block_after

That's a good point. Then we probably need a new helper. I'm just telling you what the current spec requires. If it's not possible to implement then we may need to discuss ways to deal with it with Golem team.

If there are less than SCIImplementation.REQUIRED_CONFS block overall, then no block qualify

then what? raise exception?

No. the list is empty. No transactions qualify to be included in the results. Simply, if there are less than 6 blocks, then every block is considered unconfirmed and there are no transactions.

This is just a very unlikely corner case. I specified it only because last block - SCIImplementation.REQUIRED_CONFS is negative in that situation and I decided that it's better to mention this corner case separately than to complicate the expression you need 99.9999% of the time. It would only happen if Ethereum team decided to run a new testnet and Concent connected to it immediately.