rpc: add includePending option to name_list#586
Open
mstrofnone wants to merge 3 commits into
Open
Conversation
When wallet support is enabled, MaybeWalletForRequest is meant to attach the active wallet to a request so that name_show, name_history, name_scan and name_pending can decorate their results with an "ismine" field. The original implementation gated the wallet lookup on util::AnyPtr<wallet::WalletContext>(request.context) called from src/rpc/names.cpp, which lives in the bitcoin_node static archive. On platforms where typeinfo for wallet::WalletContext ends up duplicated across the bitcoin_node and bitcoin_wallet archives (notably macOS clang with the default hidden-typeinfo visibility), the typeid stored in request.context (set in src/wallet/interfaces.cpp inside bitcoin_wallet) does not compare equal to the typeid resolved from the node-side TU, even though the demangled type name string is identical. As a result, the AnyPtr check returned nullptr, MaybeWalletForRequest fell through to "no wallet", and ismine never appeared on those RPCs. Move the wallet lookup into a new wallet-side helper, MaybeGetWalletForJSONRPCRequest, so the typeid comparison happens in the same translation unit that originally stored the WalletContext pointer. The helper also collapses the "no wallet available" cases (no wallet context, no wallet loaded, multiple wallets without selection) into a single null return, which is exactly the contract the existing callers in MaybeWalletForRequest already wanted. Verified locally on macOS arm64: name_pending.py, name_list.py, name_ismine.py, and the rest of the name_*.py functional tests all pass after this change, and name_show / name_scan again populate ismine for wallet-owned names.
Add a {"mineOnly": true} option to the name_pending RPC that filters
the result to only the unconfirmed name operations whose output address
is owned by the loaded wallet. The default is false, preserving the
existing behavior of returning every pending name operation in the
mempool.
Without this option, a wallet UI that polls name_pending to track its
own pending registrations and updates has to walk every pending name
operation in the mempool on every poll and discard most of them client
side. That is fine on a quiet chain but scales linearly with overall
pending name traffic. With mineOnly:true the filter happens server
side, before getNameInfo / addOwnershipInfo run, so the wallet pays
roughly O(its own pending ops) instead of O(all pending name ops in
the mempool).
The new option requires a wallet to be loaded. When ENABLE_WALLET is
defined and no wallet is loaded (or the request is otherwise
ambiguous, e.g. multiple wallets with no /wallet/<name> selection),
the call returns RPC_WALLET_NOT_FOUND with a clear message rather than
silently returning everything. When ENABLE_WALLET is undefined,
mineOnly:true returns RPC_METHOD_NOT_FOUND.
Also adds test/functional/name_pending_mineonly.py covering:
* mineOnly:false matches the default,
* mineOnly:true with a self-owned pending update,
* a transferred pending update flips ownership across wallets,
* mineOnly composes with the name filter and the "does not exist"
case,
* mineOnly without a wallet errors RPC_WALLET_NOT_FOUND.
Adds a {"includePending": true} option to the name_list RPC that
also includes unconfirmed wallet name operations from the mempool in
the result. The default remains false, preserving existing behavior.
Motivation: today a wallet UI that wants "my names, current state"
has to call name_list (confirmed only) AND name_pending separately,
then merge them client side. Every Namecoin wallet UI ends up
duplicating that merge logic, including the QT name table model in
this repo (src/qt/nametablemodel.cpp).
When includePending is set, name_list walks pwallet->mapWallet for
unconfirmed wallet txs in addition to confirmed ones. For each
unconfirmed name op:
* the entry is decorated with "pending": true,
* "op" is set to either "name_firstupdate" or "name_update",
* a pending entry replaces any confirmed entry for the same name,
since the wallet's pending op is its more recent intended state,
* abandoned wallet txs are skipped.
If multiple pending wallet txs touch the same name (e.g. an RBF
chain), the one with the highest nOrderPos wins, which is the most
recently created of the wallet's own pending txs.
The result schema marks height / expires_in / expired as optional
because they are omitted for pending entries (they are meaningless
without a confirmed chain height). The new "pending" and "op"
fields are also optional and only appear on unconfirmed entries.
The output for confirmed entries when includePending is unset is
byte-for-byte unchanged.
Also adds test/functional/name_list_pending.py covering:
* the default behavior is unchanged,
* includePending without any pending tx matches the default,
* a pending name_update replaces the confirmed entry for that
name and carries pending=true, op="name_update",
* confirmed entries for other names are not affected,
* the name filter composes with includePending,
* once the pending tx confirms, the entry loses the pending /
op fields and gains real expiration data again.
Author
|
yes @domob1812 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds an
{"includePending": true}option to thename_listRPC that includes the wallet's unconfirmed name operations from the mempool alongside its confirmed names. Default isfalse, so existing callers see no change.Why
Today a wallet UI that wants "my names, current state" has to:
name_listfor the wallet's confirmed names.name_pending(now withmineOnly:truefrom rpc: add mineOnly to name_pending; fix optional ismine across static archives #585) for the wallet's pending name ops.Every Namecoin wallet UI ends up duplicating that merge logic. Even this repo's QT name table model (
src/qt/nametablemodel.cpp) does the same dance:With
includePending:truethe merge happens server-side, in the place that already has both pieces of data and the right locks.Behavior
includePending:falsepreserves the previous behavior byte-for-byte.name_listalso walkspwallet->mapWalletfor unconfirmed wallet txs (depth <= 0) and emits an entry per name op."pending": true"op": either"name_firstupdate"or"name_update"nOrderPoswins — i.e. the most recently created of the wallet's own pending txs.tx.isAbandoned()pending txs are skipped, mirroring howpwallet->mapWalletconsumers usually treat them.Schema notes
Pending entries have no canonical chain height, so the existing
height,expires_in, andexpiredfields are omitted for them. The result schema now marks those three fields as optional. The output for confirmed entries (with or withoutincludePending) is byte-for-byte unchanged.The new
pendingandopfields are also optional and only appear on unconfirmed entries.Companion changes
This PR is the third leg of three closely-related changes:
mineOnlytoname_pendingand fixes the underlyingMaybeWalletForRequest/typeid-duplication bug soname_pending,name_show,name_history, andname_scanactually populateismineon platforms wherewallet::WalletContexttypeinfo gets duplicated across the static node and wallet libraries (notably macOS).includePendingtoname_list.pending/opfield names in electrum-nmc'sname_listdaemon command output. The field names here are picked to match so wallet UIs written against either backend can share parsing.This branch is currently stacked on top of #585. The two name-list-side changes (the underlying ismine fix and
includePending) are independent, but stacking lets reviewers see the full picture and lets me share the same test infrastructure between the two PRs. If reviewers prefer this PR rebased ontomasterindependently, I'll do it.Tests
A new functional test,
test/functional/name_list_pending.py, covers:name_listbehavior is unchanged,includePendingwithout any pending wallet tx matches the default,name_updatereplaces the confirmed entry for that name and carriespending=true,op="name_update",includePending,I ran the full
name_*.pybattery locally on macOS arm64 with all three commits applied — all 28 tests pass:Build: macOS 15 arm64, clang from Xcode toolchain, CMake build with default flags,
ENABLE_WALLET=ON.RFC notes / questions for review
This is shaped as something close to a draft; happy to iterate on:
nOrderPos(most-recently-added to the wallet). An alternative is "first one found in the mempool that's not RBF-replaced."nOrderPoskeeps the implementation entirely wallet-side without needing to consult the mempool again, and matches what a UI would intuitively want ("show me my latest intent"), but I can switch if reviewers prefer the mempool-anchored interpretation.mergePending. That said, every wallet UI I've seen wants the merged view, and the schema cost of adding two rows for one name is non-trivial.pwallet->mapWallet. It does not include "someone else broadcast a tx that transfers a name to my wallet but the tx isn't in my wallet yet." Adding that would require touching the mempool fromname_list, which I'd rather do as a separate change if at all.