rpc: getorphantxs follow-up

tdb3 commented at 0:49 am on October 7, 2024: contributor

Implements follow-up suggestions from #30793.

Now disallows undefined verbosity levels (below and above valid values) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786093549)
Disallows boolean verbosity (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) and adds guidance to developer-notes
Checks that getorphantxs is a hidden rpc (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786107786)
Adds a test for expiration time
Adds entry time to the returned orphan objects (verbosity >=1) to relieve the user from having to calculate it from expiration. Also adds associated test. (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1743687732)
Minor cleanup (blank line removal and log message move) (https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786092641)

Included a commit to rename the test to a more generic get_orphans to better accommodate future orphanage-related RPCs (e.g. getorphanangeinfo). Can drop the refactor commit from this PR if people feel strongly about it.

DrahtBot commented at 0:49 am on October 7, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, rkrux, itornaza, achow101
Concept ACK	instagibbs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30239 (Ephemeral Dust by instagibbs)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label RPC/REST/ZMQ on Oct 7, 2024

tdb3 force-pushed on Oct 7, 2024

tdb3 marked this as ready for review on Oct 7, 2024

in test/functional/rpc_orphans.py:120 in 5d971884cb outdated

115@@ -105,6 +116,9 @@ def test_orphan_details(self):
116         assert_equal(len(node.getorphantxs()), 1)
117         orphan_1 = orphanage[0]
118         self.orphan_details_match(orphan_1, tx_child_1, verbosity=1)
119+        self.log.info("Checking orphan entry/expiration times")
120+        assert_approx(orphan_1["entry"], entry_time, 5)

maflcko commented at 6:16 am on October 7, 2024:

I wonder if it is possible to use mocktime and an exact assert?

tdb3 commented at 11:27 am on October 7, 2024:

Yeah, if we can, that would be simpler/preference.

Would need the overhead (message sending, processing, orphanage addition, etc.) to always take less than a second, right? Otherwise there might be intermittent test failure.

If it’s always under a second, then maybe something like setmocktime(int(time.time())) to force time to start at the beginning of a second might work?

Locally, I very roughly checked runtime and it was consistently under 105ms (not sure how accurate logger is though). Any good way to benchmark on a less optimistic environment (e.g. congested CI)?

 0diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py
 1index 207647f548..294a73066b 100755
 2--- a/test/functional/rpc_orphans.py
 3+++ b/test/functional/rpc_orphans.py
 4@@ -96,11 +96,14 @@ class OrphanRPCsTest(BitcoinTestFramework):
 5         tx_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_2["new_utxo"])
 6         peer_1 = node.add_p2p_connection(P2PInterface())
 7         peer_2 = node.add_p2p_connection(P2PInterface())
 8+        self.log.info("Setting time to the start of a second")
 9         entry_time = int(time.time())
10+        node.setmocktime(entry_time)
11         peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
12         peer_2.send_and_ping(msg_tx(tx_child_2["tx"]))
13 
14         orphanage = node.getorphantxs(verbosity=2)
15+        self.log.info(f"Just after first getorphantxs, entry_time={entry_time}")
16         assert tx_in_orphanage(node, tx_child_1["tx"])
17         assert tx_in_orphanage(node, tx_child_2["tx"])

02024-10-07T11:18:24.529000Z TestFramework (INFO): Setting time to the start of a second
12024-10-07T11:18:24.632000Z TestFramework (INFO): Just after first getorphantxs, entry_time=1728299904

maflcko commented at 11:32 am on October 7, 2024:

I think with mocktime, you don’t have to do benchmarks. In most cases it should work on any system, or not at all.

Mocktime applies to most times, so there is no requirement that something happens in less than 105ms.

You should be able to test this by adding a time.sleep(1.1) and observing that the test still passes.

tdb3 commented at 11:58 am on October 7, 2024:

Thanks.

The following passed. Will push.

 0diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py
 1index 207647f548..825657a1ed 100755
 2--- a/test/functional/rpc_orphans.py
 3+++ b/test/functional/rpc_orphans.py
 4@@ -96,11 +96,15 @@ class OrphanRPCsTest(BitcoinTestFramework):
 5         tx_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_2["new_utxo"])
 6         peer_1 = node.add_p2p_connection(P2PInterface())
 7         peer_2 = node.add_p2p_connection(P2PInterface())
 8+        self.log.info("Setting time to the start of a second")
 9         entry_time = int(time.time())
10+        node.setmocktime(entry_time)
11+        time.sleep(3)
12         peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
13         peer_2.send_and_ping(msg_tx(tx_child_2["tx"]))
14 
15         orphanage = node.getorphantxs(verbosity=2)
16+        self.log.info(f"Just after first getorphantxs, entry_time={entry_time}")
17         assert tx_in_orphanage(node, tx_child_1["tx"])
18         assert tx_in_orphanage(node, tx_child_2["tx"])
19 
20@@ -117,8 +121,8 @@ class OrphanRPCsTest(BitcoinTestFramework):
21         orphan_1 = orphanage[0]
22         self.orphan_details_match(orphan_1, tx_child_1, verbosity=1)
23         self.log.info("Checking orphan entry/expiration times")
24-        assert_approx(orphan_1["entry"], entry_time, 5)
25-        assert_approx(orphan_1["expiration"], entry_time + ORPHAN_MAX_RETENTION_TIME, 5)
26+        assert_equal(orphan_1["entry"], entry_time)
27+        assert_equal(orphan_1["expiration"], entry_time + ORPHAN_MAX_RETENTION_TIME)
28 
29         self.log.info("Checking orphan details (verbosity 2)")
30         orphanage = node.getorphantxs(verbosity=2)

tdb3 force-pushed on Oct 7, 2024

tdb3 commented at 12:00 pm on October 7, 2024: contributor

Pushed to ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9 to replace assert_approx with assert_equal (using setmocktime)

in test/functional/test_framework/mempool_util.py:22 in ae17f9050e outdated

18@@ -19,6 +19,8 @@
19     MiniWallet,
20 )
21 
22+ORPHAN_MAX_RETENTION_TIME = 1200

instagibbs commented at 3:02 pm on October 7, 2024:

let’s name it the same as in the CPP code? ORPHAN_TX_EXPIRE_TIME

tdb3 commented at 9:46 pm on October 25, 2024:

fixed

instagibbs commented at 3:11 pm on October 7, 2024: member

Pushed to https://github.com/bitcoin/bitcoin/commit/ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9 to replace assert_approx with assert_equal (using setmocktime)

good call, approx always sets off alarm bells as a test reader :)

we have a chance to outright reject verbosity>2 for extensibility before a release, but I can’t think of what other things you’d possible want that can’t be given in the 3 modes already included.

concept ACK

itornaza approved

itornaza commented at 11:29 am on October 10, 2024: contributor

Concept ACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9

Enriches the functional tests for the the getorphantxs rpc and offers added functionality to conveniently access the expiration times of transactions in the orphanage.

Broadening the context with refactoring from rpc_getorphantxs.py to rpc_orphans.py in 469cbac75c088dcb0e3dc5fe8ca7f034148b9487 as a future provision for more orphanage rpcs looks good to me.

rkrux approved

rkrux commented at 4:52 am on October 23, 2024: none

tACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9

Successful make and functional tests. Pretty straightforward PR; I agree with @instagibbs’s suggestion to keep the variable name in functional test same as in the CPP code.

DrahtBot requested review from itornaza on Oct 23, 2024

DrahtBot requested review from instagibbs on Oct 23, 2024

instagibbs changes_requested

instagibbs commented at 2:10 pm on October 23, 2024: member

I really think ORPHAN_MAX_RETENTION_TIME should be calledORPHAN_TX_EXPIRE_TIME

after that LGTM

DrahtBot requested review from instagibbs on Oct 23, 2024

maflcko commented at 3:04 pm on October 23, 2024: member

Would be good to address #30793 (review) as well. Otherwise there will be another follow-up?

tdb3 commented at 4:17 pm on October 23, 2024: contributor

Would be good to address #30793 (comment) as well. Otherwise there will be another follow-up?

I agree that this is a worthwhile update. I’ll take a deeper look and see if it might fit nicely into this PR (for less code churn). If not, then it could become its own PR.

I really think ORPHAN_MAX_RETENTION_TIME should be calledORPHAN_TX_EXPIRE_TIME

Updated locally. Will push after looking into the above.

we have a chance to outright reject verbosity>2 for extensibility before a release, but I can’t think of what other things you’d possible want that can’t be given in the 3 modes already included.

Is the preference to reject undefined verbosity values for new code (e.g. -1, 5)? If so, we can adjust that and any future verbosity levels would be accompanied by updated test(s).

instagibbs commented at 3:41 pm on October 25, 2024: member

Is the preference to reject undefined verbosity values for new code (e.g. -1, 5)? If so, we can adjust that and any future verbosity levels would be accompanied by updated test(s).

Rejection now means we can expand usage later if desired, and is more obvious what’s happening.

rpc: disallow undefined verbosity in getorphantxs ac68fcca70

test: check that getorphantxs is hidden 7824f6b077

refactor: rename rpc_getorphantxs to rpc_orphans

Generalizes the test to accommodate additional
orphan-related RPCs

56bf302714

rpc: add entry time to getorphantxs 808a708107

test: add entry and expiration time checks 63f5e6ec79

tdb3 force-pushed on Oct 25, 2024

tdb3 commented at 9:45 pm on October 25, 2024: contributor

0ea84bc362f395fd247623c22942eb5ca3d1b874

Renamed expiration variable to ORPHAN_TX_EXPIRE_TIME
getorphantxs now rejects undefined verbosity (<0 and >2), and disallows boolean verbosity through an updated ParseVerbosity().

rpc: disallow boolean verbosity in getorphantxs

Updates ParseVerbosity() to support disallowing
boolean verbosity.  Removes boolean verbosity
for getorphantxs to encourage integer verbosity
usage

698f302df8

doc: add rpc guidance for boolean verbosity avoidance 7a2e6b68cd

test: explicitly check boolean verbosity is disallowed 0ea84bc362

tdb3 force-pushed on Oct 25, 2024

glozow commented at 3:03 pm on October 26, 2024: member

utACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

DrahtBot requested review from rkrux on Oct 26, 2024

rkrux approved

rkrux commented at 10:40 am on October 27, 2024: none

tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

itornaza approved

itornaza commented at 10:45 am on October 27, 2024: contributor

tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

Tested against all of the standard and extended test harness. Everything looks great!

achow101 commented at 6:19 pm on October 29, 2024: member

ACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

achow101 merged this on Oct 29, 2024

achow101 closed this on Oct 29, 2024

rpc: getorphantxs follow-up #31043

Code Coverage

Reviews

Conflicts