search

pybitcash / bitcash

BitCash: Python Bitcoin Cash Library (fork of ofek's Bit)
https://bitcash.dev
MIT License
97 stars 39 forks source link

Adds test for blockheight for all endpoints #141

Closed yashasvi-ranawat closed 4 months ago

yashasvi-ranawat commented 4 months ago

A method is added to NetworkAPI that sanitizes the endpoints; where the endpoints that are unreachable or are un-synced (based on their given blockheight) are removed.

This fixes issues like #140

yashasvi-ranawat commented 4 months ago

Why didn't the tests trigger?

merc1er commented 4 months ago

Why didn't the tests trigger?

Good question. We've always had this issue I believe. The tests are supposed to run on your fork. I'll see if there's some settings preventing this in this repo.

merc1er commented 4 months ago

Settings look OK...

Screenshot 2024-04-22 at 10 08 07

Anyway, I'll review and test locally for now.

ssmycelium commented 4 months ago

This is very clever! Nice work!

yashasvi-ranawat commented 4 months ago

I'm thinking a way to mitigate this would be to cache the provider so the block height is not fetched every time.

I implemented a time-to-live cache for blockheight with cache time limit of 120s.

Implementation Wall time
No blockheight based sanitation 365ms
Cached blockheight, first hit 2.71s
Cached blockheight, second hit 1.05s
Cached blockheight, no unresponsive bitcoin.com api, first hit 1.34s
Cached blockheight, no unresponsive bitcoin.com api, second hit 514ms

We can remove the second default API in BitcoinDotComAPI, which is the, now unresponsive, rest.bitcoin.com. It causes much delay in blockheight retrieval.

yashasvi-ranawat commented 4 months ago

oh python 3.8 functools does not have cache! Although it'll be at its end-of-life this October; don't know if I want to implement a custom cache for it.

merc1er commented 4 months ago

Thanks! Yes we can definitely remove rest.Bitcoin.com and drop Python 3.8 support.

However, there is already a caching system present in BitCash for currency conversion support: set_rate_cache_time

Why not use something like this instead? Just a question, no need to re-implement everything. I can submit a PR removing rest.Bitcoin.com in the meantime.

merc1er commented 4 months ago

@yashasvi-ranawat I have prepared a PR removing rest.Bitcoin.com. Feel free to review it if you have the time (I completely understand if not; in which case I will merge).

https://github.com/pybitcash/bitcash/pull/143

yashasvi-ranawat commented 4 months ago

However, there is already a caching system present in BitCash for currency conversion support: set_rate_cache_time

yeah, good idea! I'll have a look.

yashasvi-ranawat commented 4 months ago

I had a look at the cache setup for rates API! It was similar to the one I added to utils, but was specific to use for rates API. I refactored the one in utils to cache multiple entries with LRU cache. This is now usable by both blockheight caching and rates caching. I updated the tests accordingly.

yashasvi-ranawat commented 4 months ago
I further refactored to caching of sanitized endpoints, instead of endpoint's blockheight. This drastically improves wall time. Implementation Wall time
No blockheight based sanitisation 365 ms
Cached sanitized endpoints, first hit 1.28 s
Cached sanitized endpoints, second hit 198 ms

This PR is ready for review!