feat(qt): add support for reporting OP_RETURN payloads as Data Transactions#7144
feat(qt): add support for reporting OP_RETURN payloads as Data Transactions#7144PastaPastaPasta merged 1 commit intodashpay:developfrom
OP_RETURN payloads as Data Transactions#7144Conversation
kwvg
changed the title
OP_RETURN payloads as Data TransactionsOP_RETURN payloads as Data Transactions
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds DataTransaction support across the Qt transaction UI. It introduces TransactionRecord::DataTransaction and TransactionRecord::IsDataScript, creates DataTransaction records when encountering OP_RETURN scripts during transaction processing, updates TransactionTableModel and TransactionView to label and filter DataTransaction, and extends transaction HTML rendering to extract and display OP_RETURN payload hex in transaction descriptions. Sequence Diagram(s)sequenceDiagram
participant Backend as Wallet/Backend
participant Recorder as TransactionRecord
participant Model as TransactionTableModel
participant View as TransactionView (UI)
participant Desc as TransactionDesc (HTML renderer)
Backend->>Recorder: process transaction inputs/outputs
alt script is OP_RETURN
Recorder->>Recorder: create DataTransaction record
end
Recorder->>Model: add/update record
Model->>View: provide rows for display
View->>User: show "Data Transaction" in list
User->>View: select transaction
View->>Desc: request toHTML for record
Desc->>Desc: detect OP_RETURN, extract payload pushes
Desc->>View: return HTML with Payload: <hex>
View->>User: display transaction details with payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WalkthroughThe changes introduce support for displaying and filtering OP_RETURN (data) transactions across the Qt wallet interface. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
nvm |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/qt/bitcoingui.cpp`:
- Around line 1094-1095: The call to updateWidth() after
setWalletActionsEnabled(false) causes disabled-but-still-visible toolbar buttons
to be excluded from the minimum-width calculation; either remove this
updateWidth() invocation (so disabled state doesn't affect width) or modify
updateWidth() itself to compute width using isVisible() rather than isEnabled();
locate updateWidth(), setWalletActionsEnabled(), and the resize/setMinimumWidth
usage and implement the chosen fix so the minimum width is consistent with
visible buttons (avoid changing resize behavior here).
In `@src/qt/masternodemodel.cpp`:
- Around line 288-304: The EditRole currently returns 0 for banned nodes with
unknown ban time causing them to bypass the "Hide Banned" filter; modify the
return in masternodemodel.cpp where m_current_height > 0 and entry->isBanned()
is true but entry->poseBanHeight() is null/invalid to return a small positive
sentinel (e.g., 1) instead of 0 so the filter (which checks status_value > 0)
will hide them and avoid colliding with active nodes whose computed negative
status can be zero; update the logic around entry->poseBanHeight(),
m_current_height, and the banned branch of the EditRole method to return 1 for
unknown-ban-time cases.
🧹 Nitpick comments (1)
src/qt/transactionrecord.h (1)
22-23: Redundant forward declaration ofCScript.
CScriptis already fully defined via#include <key_io.h>(line 10), which transitively includesscript/script.h. The forward declaration on line 22 is unnecessary since the inlineIsDataScriptmethod on lines 99–101 requires a complete type anyway.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/qt/transactiondesc.cpp (1)
299-320: Clean implementation — one minor design note on multi-push concatenation.The extraction logic is correct:
begin() + 1properly skips the single-byteOP_RETURNopcode, andGetOpcorrectly accumulates only data-push contents into the result.Note that if an
OP_RETURNoutput contains multiple separate data pushes (e.g.,OP_RETURN <push1> <push2>), they are silently concatenated into a single hex string without any delimiter. This makes it impossible for the user to distinguish the push boundaries. If that's intentional (protocol-specific payloads are typically consumed as a blob), this is fine. If distinguishing individual pushes matters, consider inserting a separator (e.g., a space) between them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/transactiondesc.cpp` around lines 299 - 320, The current payload extractor (the lambda in the TransactionRecord::DataTransaction handling that skips OP_RETURN and uses script.GetOp to collect vch into ret) concatenates multiple data pushes into one contiguous hex string, losing push boundaries; modify the payload construction in that lambda to collect each vch as a separate element (e.g., vector<string> or vector<vector<uint8_t>>) and then when building strHTML use HexStr on each push and join them with a clear separator (e.g., a space or comma) instead of HexStr(ret) so the output shows individual pushes; keep TransactionRecord::IsDataScript and GetOp usage but change how their vch results are aggregated and presented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/qt/transactiondesc.cpp`:
- Around line 299-320: The current payload extractor (the lambda in the
TransactionRecord::DataTransaction handling that skips OP_RETURN and uses
script.GetOp to collect vch into ret) concatenates multiple data pushes into one
contiguous hex string, losing push boundaries; modify the payload construction
in that lambda to collect each vch as a separate element (e.g., vector<string>
or vector<vector<uint8_t>>) and then when building strHTML use HexStr on each
push and join them with a clear separator (e.g., a space or comma) instead of
HexStr(ret) so the output shows individual pushes; keep
TransactionRecord::IsDataScript and GetOp usage but change how their vch results
are aggregated and presented.
|
Addressed the two actionable items from CodeRabbit's review in kwvg#2:
🤖 This was generated by an automated review bot. |
|
@thepastaclaw both comments stem from diffs in dash#7147 that aren't in this PR. |
|
Good catch — sorry about the confusion! Those were indeed from #7147's diff, not this PR. I'll be more careful about which PR context I'm reviewing in. 👍 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/qt/transactionrecord.h (1)
22-23: Include<script/script.h>directly to support the inlineIsDataScriptmethod.The inline
IsDataScriptmethod (line 99–101) calls member functions (empty(),operator[]) onCScriptand uses theOP_RETURNconstant, requiring the fullCScriptdefinition. Although the code currently compiles due to a transitive include chain (key_io.h→script/standard.h→script/interpreter.h→primitives/transaction.h→script/script.h), this dependency is fragile and implicit. Replace the forward declaration with a direct include of<script/script.h>to make the dependency explicit and clearer.♻️ Proposed fix
-class CScript; +#include <script/script.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/transactionrecord.h` around lines 22 - 23, The file declares CScript via a forward declaration but the inline method IsDataScript in transactionrecord.h uses CScript methods (empty(), operator[]) and OP_RETURN, so replace the forward declaration with a direct include of <script/script.h>; update transactionrecord.h to `#include` <script/script.h> and remove the standalone "class CScript;" so IsDataScript has the full CScript definition and OP_RETURN symbol available for compilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/transactiondesc.cpp`:
- Around line 300-319: The loop currently iterates all outputs (wtx.tx->vout)
and shows every OP_RETURN payload for a DataTransaction; instead restrict
payload extraction to the specific output referenced by this TransactionRecord
(rec->idx). Replace the for-loop with logic that checks rec->idx is within
wtx.tx->vout bounds, use that single TxOut (wtx.tx->vout[rec->idx]), verify
TransactionRecord::IsDataScript(scriptPubKey) and only then run the existing
payload-extraction and HTML append; keep the same payload extraction lambda and
HexStr usage but scoped to that one output.
---
Nitpick comments:
In `@src/qt/transactionrecord.h`:
- Around line 22-23: The file declares CScript via a forward declaration but the
inline method IsDataScript in transactionrecord.h uses CScript methods (empty(),
operator[]) and OP_RETURN, so replace the forward declaration with a direct
include of <script/script.h>; update transactionrecord.h to `#include`
<script/script.h> and remove the standalone "class CScript;" so IsDataScript has
the full CScript definition and OP_RETURN symbol available for compilation.
Good point too imo |
|
Addressed the two CodeRabbit items confirmed by @UdjinM6 in kwvg#3:
🤖 This was generated by an automated review bot. |
kwvg
requested review from
PastaPastaPasta and
UdjinM6
and removed request for
UdjinM6
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/transactionrecord.h (1)
73-92:⚠️ Potential issue | 🟠 MajorAdd
DataTransactiontoEXCLUDED_TYPESinTransactionFilterProxy.The inline comment at line 73 requires
EXCLUDED_TYPESto be updated when adding a newTypeenum value.DataTransactionwas added to the enum but is missing fromEXCLUDED_TYPESinsrc/qt/transactionfilterproxy.h(lines 28–34). This causesDataTransactionto be included by default in common transaction filters, which may be unintended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/transactionrecord.h` around lines 73 - 92, The new enum value DataTransaction was added to the TransactionRecord::Type enum but not added to the EXCLUDED_TYPES list used by TransactionFilterProxy, so DataTransaction is being included by default; update the EXCLUDED_TYPES constant in TransactionFilterProxy to include DataTransaction (add the exact symbol DataTransaction to the excluded types array/initializer alongside the other enum names) so it is filtered the same as the other excluded transaction types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/transactionrecord.cpp`:
- Around line 105-113: The DataTransaction credit branch leaves
TransactionRecord::involvesWatchAddress uninitialized, causing undefined
behavior; explicitly set the flag to false before appending (e.g., in the block
that creates sub for IsDataScript) or alternatively ensure the
TransactionRecord(uint256, qint64) constructor initializes involvesWatchAddress
to false; locate the branch creating sub (TransactionRecord sub(hash, nTime);
... sub.type = TransactionRecord::DataTransaction; parts.append(sub);) and
assign sub.involvesWatchAddress = false prior to parts.append(sub).
---
Outside diff comments:
In `@src/qt/transactionrecord.h`:
- Around line 73-92: The new enum value DataTransaction was added to the
TransactionRecord::Type enum but not added to the EXCLUDED_TYPES list used by
TransactionFilterProxy, so DataTransaction is being included by default; update
the EXCLUDED_TYPES constant in TransactionFilterProxy to include DataTransaction
(add the exact symbol DataTransaction to the excluded types array/initializer
alongside the other enum names) so it is filtered the same as the other excluded
transaction types.
---
Duplicate comments:
In `@src/qt/transactiondesc.cpp`:
- Around line 299-317: No change needed — the OP_RETURN payload handling in the
TransactionRecord DataTransaction branch is correct: it revalidates with
TransactionRecord::IsDataScript on wtx.tx->vout[rec->idx], safely bounds-checks
rec->idx, skips the initial opcode in the payload lambda, accumulates data
pushes using script.GetOp into the payload vector, and outputs a hex-only string
via HexStr; leave the TransactionRecord/DataTransaction block, the payload
lambda, and the HexStr output as-is.
Proposals are made in the form of data transactions but look like oddly shaped transactions in the UI, this should help better explain where the proposal fee went.
…ayloads as Data Transactions 5c22389 qt: add support for reporting `OP_RETURN` payloads as Data Transactions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on dashpay#7147 | `develop` (06e761e) | This PR (97ae689) | | ---------------------- | -------------------- | |  |  | |  |  | ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 5c22389 UdjinM6: utACK 5c22389 Tree-SHA512: 37ade0aed7f64885268551cdab79c8c966508792d63e74c97a4f622b834cde74954d7c5e3b08b7ae35b38122270f148bff27935290de08644b0a72be81cb5127
00f590d Merge #7180: qt: add Tahoe styled icons for macOS, runtime styling for each network type, update bundle icon, add mask-based tray icon, generation scripts (pasta) 60dda51 Merge #7176: perf: do linear lookup instead building 2 heavy Hash-Maps (pasta) df1ca87 Merge #7159: feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) (pasta) 9061ad0 Merge #7118: feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) (pasta) 64cc4f2 Merge #7160: feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics (pasta) 5d28a69 Merge #7157: fix(qt): prevent banned masternodes from returning status=0 (pasta) e0b7386 Merge #7146: feat(qt): introduce framework for sourcing and applying data, use for `{Masternode,Proposal}List`s (pasta) 8fd53cd Merge #7144: feat(qt): add support for reporting `OP_RETURN` payloads as Data Transactions (pasta) cc6f0bb Merge #7154: fix: MN update notifications had old_list/new_list swapped (pasta) 33f0138 Merge #7145: fix(qt): move labelError styling from proposalcreate.ui into general.css (pasta) 1bdbde6 Merge #7148: feat(qt): persist filter preferences in masternode list (pasta) 96bb601 Merge #7147: fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering (pasta) Pull request description: ## Backport PRs for v23.1.1 Cherry-picks the following 12 PRs (labeled `backport-candidate-23.1.x`) from `develop` onto `v23.1.x`, in merge order: | PR | Title | |---|---| | #7147 | fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering | | #7148 | feat(qt): persist filter preferences in masternode list | | #7145 | fix(qt): move labelError styling from proposalcreate.ui into general.css | | #7154 | fix: MN update notifications had old_list/new_list swapped | | #7144 | feat(qt): add support for reporting `OP_RETURN` payloads as Data Transactions | | #7146 | feat(qt): introduce framework for sourcing and applying data, use for `{Masternode,Proposal}List`s | | #7157 | fix(qt): prevent banned masternodes from returning status=0 | | #7160 | feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics | | #7118 | feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) | | #7159 | feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) | | #7176 | perf: do linear lookup instead building 2 heavy Hash-Maps | | #7180 | qt: add Tahoe styled icons for macOS, runtime styling for each network type, update bundle icon, add mask-based tray icon, generation scripts | All 12 cherry-picked cleanly (no conflicts). ## Notes - Used `git cherry-pick -m 1 <merge-sha>` for each (all were merge commits on develop) - Applied in chronological merge order to respect dependency chains - Version bump and release notes are separate steps per the release process ACKs for top commit: kwvg: utACK 00f590d UdjinM6: utACK 00f590d Tree-SHA512: 90d2a0660db8daa69b3e3b33a8a790fb0ea7d9a04656a2e27955575e76b6f4c9a379c435ef1c573ef6669c36cb6e205ba9701716d3dc303b01f19c719516b6d1
da16809 docs: add #7221 and #7222 to v23.1.2 release notes (PastaClaw) 8a93926 Merge #7222: fix: properly skip evodb repair on reindex (pasta) b74b549 Merge #7221: refactor: rename bitcoin-util manpage and test references to dash-util (pasta) 81464ac Merge #7211: fix: qt info tab layout (pasta) 7c27c2f Merge #7209: fix: serialize TrySignChainTip to prevent concurrent signing race (pasta) 81d5eb2 Merge #7208: fix: skip collecting block txids during IBD to prevent unbounded memory growth (pasta) d02243c ci: run check-skip on blacksmith with GitHub-hosted fallback (PastaClaw) 033b3fe chore: regenerate manpages for v23.1.2 (PastaClaw) ff965b5 chore: v23.1.2 release preparation (PastaClaw) 8d5936d chore: add #7191 and #7193 to v23.1.1 release notes (PastaClaw) 9f3662b chore: v23.1.1 release preparation (PastaClaw) 5dbfa98 chore: v23.1.1 release preparation (PastaClaw) 240a95f Merge #7193: fix: reject identity elements in deserialization and key generation (pasta) 444cbf2 Merge #7191: fix(qt): reseat quorum labels when new types are inserted (pasta) 00f590d Merge #7180: qt: add Tahoe styled icons for macOS, runtime styling for each network type, update bundle icon, add mask-based tray icon, generation scripts (pasta) 60dda51 Merge #7176: perf: do linear lookup instead building 2 heavy Hash-Maps (pasta) df1ca87 Merge #7159: feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) (pasta) 9061ad0 Merge #7118: feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) (pasta) 64cc4f2 Merge #7160: feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics (pasta) 5d28a69 Merge #7157: fix(qt): prevent banned masternodes from returning status=0 (pasta) e0b7386 Merge #7146: feat(qt): introduce framework for sourcing and applying data, use for `{Masternode,Proposal}List`s (pasta) 8fd53cd Merge #7144: feat(qt): add support for reporting `OP_RETURN` payloads as Data Transactions (pasta) cc6f0bb Merge #7154: fix: MN update notifications had old_list/new_list swapped (pasta) 33f0138 Merge #7145: fix(qt): move labelError styling from proposalcreate.ui into general.css (pasta) 1bdbde6 Merge #7148: feat(qt): persist filter preferences in masternode list (pasta) 96bb601 Merge #7147: fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering (pasta) da1e336 build: expand minimum Darwin version to macOS 11 (Big Sur) (Kittywhiskers Van Gogh) Pull request description: ## Issue being fixed or feature implemented Note: Skipping changes from #7149 which was for the v23.1.x only. ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 36988f9 Tree-SHA512: f5bf8f0af11379bbcea606108ee90af08c16f588ebdbfac1fdd567adfcad14926b9c797c8fa6b398fb65fc3210c5f2c084015ea07718371df04e2412625f42d4
4 participants
Additional Information
SERVICEandSTATUSsorting, fix common types filtering #7147develop(06e761e)Breaking Changes
None expected.
Checklist