Fee Rate Improvements Part 1 #17668

aqk · 2024-03-07T18:26:21Z

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

github-actions · 2024-03-15T17:59:23Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

.github/workflows/pre-commit.yml

github-actions · 2024-03-18T16:00:32Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2024-03-25T15:39:35Z

File	Coverage	Missing Lines
`chia/_tests/wallet/test_fee_store.py`	96.7%	lines 79, 84
`chia/wallet/fee_record.py`	96.9%	lines 20
`chia/wallet/wallet_fee_rate.py`	0.0%	lines 1, 3, 5, 7-9, 12-15, 20-21, 23-24
`chia/wallet/wallet_fee_store.py`	76.1%	lines 45, 119-121, 125-130, 133, 135-136, 140-143

Total	Missing	Coverage
178 lines	34 lines	80%

coveralls-official · 2024-03-25T15:47:02Z

Pull Request Test Coverage Report for Build 8422161012

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

144 of 178 (80.9%) changed or added relevant lines in 4 files are covered.
26 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.03%) to 90.792%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
chia/wallet/fee_record.py	31	32	96.88%
chia/_tests/wallet/test_fee_store.py	59	61	96.72%
chia/wallet/wallet_fee_rate.py	0	14	0.0%
chia/wallet/wallet_fee_store.py	54	71	76.06%

Files with Coverage Reduction	New Missed Lines	%
chia/_tests/simulation/test_simulation.py	1	96.52%
chia/_tests/core/util/test_lockfile.py	1	91.09%
chia/server/node_discovery.py	1	79.96%
chia/wallet/wallet_node.py	2	88.49%
chia/full_node/weight_proof.py	2	90.73%
chia/full_node/full_node.py	4	84.63%
chia/server/server.py	4	80.0%
chia/timelord/timelord.py	11	72.49%

Totals
Change from base Build 8378992295:	-0.03%
Covered Lines:	97545
Relevant Lines:	107410

💛 - Coveralls

emlowe · 2024-03-26T22:03:45Z

@aqk There is a lot of coverage missing here - I think that needs to be fixed before this can get merged in

altendky

first thoughts

altendky · 2024-03-25T15:11:48Z

chia/wallet/wallet_fee_store.py

+MEMPOOL_CONSIDERED_FULL_RATIO = 0.8
+
+
+class FeeStore:


Make this a dataclass.

altendky · 2024-03-25T15:13:33Z

chia/wallet/wallet_fee_store.py

+        self.db_wrapper = db_wrapper
+
+        # REVIEW: Should we use two tables: one for mempool data, one for estimates?
+        async with self.db_wrapper.writer_maybe_transaction() as conn:


I wouldn't think we need the 'maybe' form here. I think that is mostly for performance critical cases?

Generally we call this a writer so it is clear what you are using at the call sites.

async with self.db_wrapper.writer_maybe_transaction() as writer:

altendky · 2024-03-25T15:26:56Z

chia/wallet/wallet_fee_rate.py

+    clvm_cost: CLVMCost
+
+    def get_rate_as_float(self) -> float:
+        return float(self.mojos) / float(self.clvm_cost)


>>> 3 / 2 1.5

Minor, but shouldn't need to float() these.

Suggested change

return float(self.mojos) / float(self.clvm_cost)

return self.mojos / self.clvm_cost

altendky · 2024-03-25T15:28:39Z

chia/wallet/wallet_fee_rate.py

+from chia.util.streamable import Streamable, streamable
+
+
+@typing_extensions.final


This is fine and we don't have a policy (I haven't even identified my own personal preference solidly yet) around when we do and don't use the extensions/backport modules, but... final is available from stdlib typing in all Python versions we support now. Leave as is or change as you prefer.

altendky · 2024-03-25T15:45:23Z

chia/wallet/wallet_fee_store.py

+
+"""
+
+MINUTES_IN_WEEK = 1440 * 7 * 60  # Approximate number of entries to keep


I think the 1440 was left over from some other calculation perhaps?

Suggested change

MINUTES_IN_WEEK = 1440 * 7 * 60 # Approximate number of entries to keep

MINUTES_IN_WEEK = 7 * 24 * 60 # Approximate number of entries to keep

Or even start with the seconds first using datetime.timedelta(weeks=1).total_seconds(). Maybe. Maybe not. I dislike the stdlib time features sufficiently that I would probably just type out the numbers.

# Approximate number of entries to keep

Is this because 18.75 seconds per block and ... do we have one transaction block for every three blocks? on average? I would personally find an actual calculation easier to follow since we (should?) have constants for the block rates. If my guessing there was even correct.

altendky · 2024-03-27T15:18:49Z

chia/wallet/wallet_fee_store.py

+        if row is not None:
+
+            return self._row_to_fee_record(row)
+        logging.getLogger(__name__).error(f"Dumping whole DB: {str()}")


Needs changed multiple places. Should probably also report what error as well as actually outputting something. Or delete.

Suggested change

logging.getLogger(__name__).error(f"Dumping whole DB: {str()}")

self.log.error(f"Dumping whole DB: {str()}")

altendky · 2024-03-27T15:19:53Z

chia/wallet/wallet_fee_store.py

+    def _row_to_fee_record(cls, row: sqlite3.Row) -> FeeRecord:
+        return FeeRecord.from_bytes(row[0])  # cast: mypy limitation
+
+    async def get_fee_record(self, key: FeeRecordKey) -> Optional[FeeRecord]:


Is this expected to need to check non-existence a lot? If not, perhaps just raising an exception when you request a not-present data point might be better.

altendky · 2024-03-27T15:21:39Z

chia/wallet/wallet_fee_store.py

+        logging.getLogger(__name__).error(f"Dumping whole DB: {str()}")
+        return None
+
+    async def get_fee_record2(self, key: FeeRecordKey) -> Optional[FeeRecord]:


If we need two methods, it would be good to clarify the difference better than 2. (I expect this is part of a temporary exploration or somesuch)

altendky · 2024-03-27T15:23:06Z

chia/wallet/wallet_fee_store.py

+    async def prune(self) -> None:
+        """Delete records older than SECONDS_IN_WEEK"""
+        async with self.db_wrapper.writer_maybe_transaction() as conn:
+            now_secs = time.time()


You may want to make the clock callable be an attribute so that you can more readily test things like this.

altendky · 2024-03-27T15:24:41Z

chia/wallet/wallet_fee_store.py

+            cursor = await conn.execute("DELETE FROM fee_records WHERE block_index>?", (block_index,))
+            await cursor.close()
+
+    async def prune(self) -> None:


Probably ought to be able to specify the age and/or time. Maybe two separate methods? I'm not exactly sure offhand what the best interface is here.

github-actions · 2024-05-12T11:05:21Z

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

aqk requested a review from a team as a code owner March 7, 2024 18:26

Fee Rate Improvements Part 1

46f76f4

aqk force-pushed the ak.fees-001 branch from 2eada54 to 46f76f4 Compare March 7, 2024 19:57

aqk added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Mar 8, 2024

github-actions bot added the coverage-diff label Mar 8, 2024

aqk added 5 commits March 12, 2024 10:36

fee tracking part 1

c2d948d

lint

fff0bbe

isort complains on CI, but is clean locally - guess

5af9b69

isort

d0771b6

Show tool versions in CI

2ae93d4

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Mar 15, 2024

OverActiveBladderSystem reviewed Mar 17, 2024

View reviewed changes

.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved

aqk requested review from altendky and emlowe March 18, 2024 15:57

Merge branch 'main' into ak.fees-001

03c83bd

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Mar 18, 2024

aqk added 3 commits March 19, 2024 10:15

Update workflows: run 'python ./tools/manage_clvm.py build'

6119b75

isort

2efca52

black

2a25dc7

aqk marked this pull request as draft March 19, 2024 17:39

aqk marked this pull request as ready for review March 19, 2024 17:39

aqk added 4 commits March 19, 2024 10:47

end-of-file fix

b02ca4c

remove workflow debugging

da2196a

lint

180bba9

lint

dc29680

aqk marked this pull request as draft March 21, 2024 17:19

aqk marked this pull request as ready for review March 21, 2024 17:19

aqk marked this pull request as draft March 21, 2024 17:26

aqk marked this pull request as ready for review March 21, 2024 17:28

force CI reset

b0c9e30

aqk closed this Mar 21, 2024

aqk reopened this Mar 21, 2024

update for tests dir move

19416ad

aqk force-pushed the ak.fees-001 branch from 2d641f2 to 19416ad Compare March 21, 2024 17:55

aqk added 5 commits March 21, 2024 11:32

lint

e619998

fix tests

bd0e4f1

typing typo

b47e149

mypy

64ec486

mypy

cb3a1a7

altendky requested changes Mar 27, 2024

View reviewed changes

github-actions bot added the stale-pr Flagged as stale and in need of manual review label May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fee Rate Improvements Part 1 #17668

Fee Rate Improvements Part 1 #17668

aqk commented Mar 7, 2024

github-actions bot commented Mar 15, 2024

github-actions bot commented Mar 18, 2024

github-actions bot commented Mar 25, 2024

coveralls-official bot commented Mar 25, 2024

emlowe commented Mar 26, 2024

altendky left a comment

altendky Mar 25, 2024

altendky Mar 25, 2024

altendky Mar 25, 2024

altendky Mar 25, 2024

altendky Mar 25, 2024

altendky Mar 27, 2024

altendky Mar 27, 2024

altendky Mar 27, 2024

altendky Mar 27, 2024

altendky Mar 27, 2024

github-actions bot commented May 12, 2024

	return float(self.mojos) / float(self.clvm_cost)
	return self.mojos / self.clvm_cost

		from chia.util.streamable import Streamable, streamable


		@typing_extensions.final


		"""

		MINUTES_IN_WEEK = 1440 * 7 * 60 # Approximate number of entries to keep

	MINUTES_IN_WEEK = 1440 * 7 * 60 # Approximate number of entries to keep
	MINUTES_IN_WEEK = 7 * 24 * 60 # Approximate number of entries to keep

	logging.getLogger(__name__).error(f"Dumping whole DB: {str()}")
	self.log.error(f"Dumping whole DB: {str()}")

		MEMPOOL_CONSIDERED_FULL_RATIO = 0.8


		class FeeStore:

Fee Rate Improvements Part 1 #17668

Are you sure you want to change the base?

Fee Rate Improvements Part 1 #17668

Conversation

aqk commented Mar 7, 2024

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

github-actions bot commented Mar 15, 2024

github-actions bot commented Mar 18, 2024

github-actions bot commented Mar 25, 2024

coveralls-official bot commented Mar 25, 2024

Pull Request Test Coverage Report for Build 8422161012

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

emlowe commented Mar 26, 2024

altendky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2024