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

Bisection search causes inconsistency in DCV values #363

Open
trizin opened this issue Nov 1, 2022 · 8 comments · May be fixed by #807
Open

Bisection search causes inconsistency in DCV values #363

trizin opened this issue Nov 1, 2022 · 8 comments · May be fixed by #807
Labels
Type: Bug Something isn't working

Comments

@trizin
Copy link
Contributor

trizin commented Nov 1, 2022

Background / Motivation

We're using bisection search to find start and end block numbers for the DF week in the networks other than ETH mainnet. As these numbers are not 100% consistent between different runs we might get different results.

There are two possible paths:

  • (a) Specify a tolerance level, that's say 1/00 of a cent worth of OCEAN. Eg 0.0001 OCEAN
  • (b) Specify a tolerance level that's within Solidity's or Python's numerical precision

Roughly, I'm ok with either. And, (b) is less risky so I recommend it, unless it's too slow.

We should be able to readily get these numbers consistent within a target tolerance level

TODOs

My suggestion to proceed: (highest level: go for b first, and if needed go for a)

  1. Write unit tests that expose the inconsistency
  2. Update the unit tests to allow a target tolerance
  3. Update bisection code to hit target tolerance
  4. If needed, update other code to allow the target tolerance too
@idiom-bytes
Copy link
Member

I think this is fixed due to the new implementation to get blocks.
@trizin can we close this issue?

@trizin
Copy link
Contributor Author

trizin commented Dec 15, 2022

No, this is a different one. The new implementation is only for mainnet, we still have this issue on other networks when getting the volumes.

@trentmc trentmc changed the title Inconsistency in DCVs Bisection search may cause inconsistency in DCV numbers May 4, 2023
@trentmc trentmc changed the title Bisection search may cause inconsistency in DCV numbers Bisection search may cause inconsistency in DCV values May 4, 2023
@trizin
Copy link
Contributor Author

trizin commented Jan 18, 2024

#793

This PR verifies that the identified block number is within a 15-minute range of the target timestamp. Throws an error otherwise.

@trentmc
Copy link
Member

trentmc commented Jan 19, 2024

#793

This PR verifies that the identified block number is within a 15-minute range of the target timestamp. Throws an error otherwise.

@trizin do you see an opportunity to "Update bisection code to hit target tolerance"? Eg by simply tightening the target tolerance, without making runtimes much longer?

@trizin
Copy link
Contributor Author

trizin commented Jan 19, 2024

@trentmc xtol is set to 0.4, which is already very low for this case. Last round the identified block numbers for Sapphire were exactly correct.

I think our timestamp to block algorithm is very efficient, doesn't take long to find the block number and finds the right block numbers too.

If something goes terribly wrong with the RPC or inputs, the 15 min check shall protect datafarming.


Getting back to the issue, this problem is particularly noticeable on networks like Polygon, where the block time is relatively short. In such cases, the bisection search might yield slightly varying results. For instance, it could return block 1001 during one execution and block 1002 in another, especially if these two blocks are almost equally close to the target timestamp.

This variability can impact the reward distribution, especially if a substantial consume occurred in block 1001. It also poses a challenge for accurate historical data generation.

Imo, the most reliable solution to mitigate these is maintaining a record of the start and end blocks for a given week (week x). For the subsequent week (week x+1), the starting block should be set as the ending block of the previous week (week x).

Or we could try:

  • xtol: Setting xtol to a much smaller value, like 0.01, would help with these inconsistencies.
  • Rounding of block numbers: Currently, the code returns 'int(block_i)' which effectively rounds down the block number. In the context of the bisection search, which may return a decimal value, using the 'round()' function to round to the nearest integer would provide more consistent results across different runs.

@trentmc
Copy link
Member

trentmc commented Jan 19, 2024

Imo, the most reliable solution to mitigate these is maintaining a record of the start and end blocks for a given week (week x). For the subsequent week (week x+1), the starting block should be set as the ending block of the previous week (week x).

Where are the candidate places that we would store the record of start & end blocks for a given week? What do you recommend. If this is reliable and repeatable, that sounds like a good choice. (Ocean Data NFT with ERC721?)

@trizin
Copy link
Contributor Author

trizin commented Jan 19, 2024

Ocean Data NFT with ERC721

Sounds brilliant! I'd propose deploying on Polygon Mainnet, it'd cost us 1 tx/week (probably around 0.01-0.03 MATIC)

@trentmc
Copy link
Member

trentmc commented Jan 19, 2024

Ocean Data NFT with ERC721

Sounds brilliant! I'd propose deploying on Polygon Mainnet, it'd cost us 1 tx/week (probably around 0.01-0.03 MATIC)

Works for me.

Here's the README in ocean.py for set/get on ERC721. But you don't need an ocean.py dependency, instead, just use its implementation as input to doing it directly.

@trentmc trentmc added the Type: Bug Something isn't working label Jan 30, 2024
@trentmc trentmc changed the title Bisection search may cause inconsistency in DCV values Bisection search causes inconsistency in DCV values Jan 30, 2024
@trizin trizin linked a pull request Feb 26, 2024 that will close this issue
@trizin trizin linked a pull request Feb 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants