rpc: getorphantxs follow-up

[tdb3]

Implements follow-up suggestions from #30793.

• Now disallows undefined verbosity levels (below and above valid values) (rpc: add getorphantxs #30793 (comment))
• Disallows boolean verbosity (rpc: add getorphantxs #30793 (comment)) and adds guidance to developer-notes
• Checks that getorphantxs is a hidden rpc (rpc: add getorphantxs #30793 (comment))
• 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. (rpc: add getorphantxs #30793 (comment))
• Minor cleanup (blank line removal and log message move) (rpc: add getorphantxs #30793 (comment))

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]

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.

[tdb3]

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

[instagibbs]

Pushed to ae17f90 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

[instagibbs]

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

[tdb3]

fixed

[itornaza]

Concept ACK ae17f90

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 469cbac as a future provision for more orphanage rpcs looks good to me.

[rkrux]

tACK ae17f90

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.

[instagibbs]

I really think ORPHAN_MAX_RETENTION_TIME should be calledORPHAN_TX_EXPIRE_TIME

after that LGTM

[maflcko]

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

[tdb3]

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]

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.

[tdb3]

0ea84bc

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

[glozow]

utACK 0ea84bc

[rkrux]

tACK 0ea84bc

[itornaza]

tACK 0ea84bc

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

[achow101]

ACK 0ea84bc