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#7144kwvg wants to merge 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.
|
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)
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]](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. 👍 |
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.
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.
| if (rec->type == TransactionRecord::DataTransaction) { | ||
| for (const auto& txout : wtx.tx->vout) { | ||
| if (!TransactionRecord::IsDataScript(txout.scriptPubKey)) { | ||
| continue; | ||
| } | ||
| const auto& script{txout.scriptPubKey}; | ||
| // Extract all data pushes after OP_RETURN | ||
| const auto payload = [&script]() { | ||
| auto pc = script.begin() + 1; // Skip opcode | ||
| opcodetype opcode; | ||
| std::vector<uint8_t> ret{}, vch{}; | ||
| while (pc < script.end() && script.GetOp(pc, opcode, vch)) { | ||
| ret.insert(ret.end(), vch.begin(), vch.end()); | ||
| } | ||
| return ret; | ||
| }(); | ||
| if (!payload.empty()) { | ||
| strHTML += "<b>" + tr("Payload") + ":</b> " + QString::fromStdString(HexStr(payload)) + "<br>"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider scoping payload display to rec->idx rather than all outputs.
The detail view is for a specific TransactionRecord (pointing to output rec->idx), but the current loop displays payloads from every OP_RETURN output in the transaction. For non-standard transactions with multiple OP_RETURN outputs this would show unrelated payloads in a single record's details pane. Standard relay limits transactions to one OP_RETURN output, so this is a minor concern today, but narrowing is more semantically correct.
🛠️ Proposed fix — scope to the specific output
- if (rec->type == TransactionRecord::DataTransaction) {
- for (const auto& txout : wtx.tx->vout) {
- if (!TransactionRecord::IsDataScript(txout.scriptPubKey)) {
- continue;
- }
- const auto& script{txout.scriptPubKey};
- // Extract all data pushes after OP_RETURN
- const auto payload = [&script]() {
- auto pc = script.begin() + 1; // Skip opcode
- opcodetype opcode;
- std::vector<uint8_t> ret{}, vch{};
- while (pc < script.end() && script.GetOp(pc, opcode, vch)) {
- ret.insert(ret.end(), vch.begin(), vch.end());
- }
- return ret;
- }();
- if (!payload.empty()) {
- strHTML += "<b>" + tr("Payload") + ":</b> " + QString::fromStdString(HexStr(payload)) + "<br>";
- }
- }
- }
+ if (rec->type == TransactionRecord::DataTransaction &&
+ rec->idx >= 0 && static_cast<size_t>(rec->idx) < wtx.tx->vout.size()) {
+ const auto& txout = wtx.tx->vout[rec->idx];
+ if (TransactionRecord::IsDataScript(txout.scriptPubKey)) {
+ const auto& script{txout.scriptPubKey};
+ // Extract all data pushes after OP_RETURN
+ const auto payload = [&script]() {
+ auto pc = script.begin() + 1; // Skip opcode
+ opcodetype opcode;
+ std::vector<uint8_t> ret{}, vch{};
+ while (pc < script.end() && script.GetOp(pc, opcode, vch)) {
+ ret.insert(ret.end(), vch.begin(), vch.end());
+ }
+ return ret;
+ }();
+ if (!payload.empty()) {
+ strHTML += "<b>" + tr("Payload") + ":</b> " + QString::fromStdString(HexStr(payload)) + "<br>";
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (rec->type == TransactionRecord::DataTransaction) { | |
| for (const auto& txout : wtx.tx->vout) { | |
| if (!TransactionRecord::IsDataScript(txout.scriptPubKey)) { | |
| continue; | |
| } | |
| const auto& script{txout.scriptPubKey}; | |
| // Extract all data pushes after OP_RETURN | |
| const auto payload = [&script]() { | |
| auto pc = script.begin() + 1; // Skip opcode | |
| opcodetype opcode; | |
| std::vector<uint8_t> ret{}, vch{}; | |
| while (pc < script.end() && script.GetOp(pc, opcode, vch)) { | |
| ret.insert(ret.end(), vch.begin(), vch.end()); | |
| } | |
| return ret; | |
| }(); | |
| if (!payload.empty()) { | |
| strHTML += "<b>" + tr("Payload") + ":</b> " + QString::fromStdString(HexStr(payload)) + "<br>"; | |
| } | |
| } | |
| if (rec->type == TransactionRecord::DataTransaction && | |
| rec->idx >= 0 && static_cast<size_t>(rec->idx) < wtx.tx->vout.size()) { | |
| const auto& txout = wtx.tx->vout[rec->idx]; | |
| if (TransactionRecord::IsDataScript(txout.scriptPubKey)) { | |
| const auto& script{txout.scriptPubKey}; | |
| // Extract all data pushes after OP_RETURN | |
| const auto payload = [&script]() { | |
| auto pc = script.begin() + 1; // Skip opcode | |
| opcodetype opcode; | |
| std::vector<uint8_t> ret{}, vch{}; | |
| while (pc < script.end() && script.GetOp(pc, opcode, vch)) { | |
| ret.insert(ret.end(), vch.begin(), vch.end()); | |
| } | |
| return ret; | |
| }(); | |
| if (!payload.empty()) { | |
| strHTML += "<b>" + tr("Payload") + ":</b> " + QString::fromStdString(HexStr(payload)) + "<br>"; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/transactiondesc.cpp` around lines 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.
There was a problem hiding this comment.
Makes sense. Transactions with multiple op_returns can't be relayed but they can be mined, should handle them correctly.
There was a problem hiding this comment.
@UdjinM6, thank you for confirming! You're absolutely right—while the standard relay policy enforces a single OP_RETURN per transaction, miners can include transactions with multiple OP_RETURN outputs in blocks. Scoping the payload display to rec->idx ensures each TransactionRecord only shows its own output's payload, which correctly handles both the standard case and the edge case of mined transactions with multiple OP_RETURN outputs.
✏️ Learnings added
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7144
File: src/qt/transactiondesc.cpp:300-319
Timestamp: 2026-02-18T18:15:45.114Z
Learning: In Dash, transactions with multiple OP_RETURN outputs cannot be relayed due to standard relay policy, but they can be mined and included in blocks. Code should handle such transactions correctly even though they are non-standard.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
Good point too imo |
|
Addressed the two CodeRabbit items confirmed by @UdjinM6 in kwvg#3:
🤖 This was generated by an automated review bot. |
4 participants
Additional Information
SERVICEandSTATUSsorting, fix common types filtering #7147develop(06e761e)Breaking Changes
None expected.
Checklist