getrawtransaction implementation

BrandonOdiwuor commented at 11:36 am on November 14, 2023: contributor

Fixes #645.

Add getrawtransaction RPC to GUI on Tools -> Verify external txid. tab

Screencast from 21-12-2023 06:35:04 ASUBUHI.webm

DrahtBot commented at 11:36 am on November 14, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, alfonsoromanz
Concept ACK	hernanmarino, kristapsk, hebasto

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

Conflicts

No conflicts as of last run.

willcl-ark commented at 11:56 am on November 14, 2023: member

Cool, that’s pretty nice!

Just thinking out loud, but it does make me wonder if perhaps a new Verify or Utils button, in line with “Send | Recieve | Transactions” could be even nicer? If we added it there, we might consider adding a few more utils to the GUI like “Decode raw transaction”, “Decode Script”, “Get Descriptor Info”, “Verify Message” etc. which folks who use the gui generally use other software for today…

BrandonOdiwuor force-pushed on Nov 14, 2023

DrahtBot added the label CI failed on Nov 14, 2023

BrandonOdiwuor commented at 2:42 pm on November 14, 2023: contributor

Cool, that’s pretty nice!

Just thinking out loud, but it does make me wonder if perhaps a new Verify or Utils button, in line with “Send | Recieve | Transactions” could be even nicer? If we added it there, we might consider adding a few more utils to the GUI like “Decode raw transaction”, “Decode Script”, “Get Descriptor Info”, “Verify Message” etc. which folks who use the gui generally use other software for today… @willcl-ark would love to hear more opinions on this, I also think Help -> Verify external txid tab might not work well with adding more utils

BrandonOdiwuor renamed this:
~~GUI getrawtransaction implementation~~
gui: getrawtransaction implementation
on Nov 14, 2023

willcl-ark commented at 2:46 pm on November 14, 2023: member

Yeah I don’t want to derail this pr, which I think looks good on its own!

If there was desire to add more utils we could always move this later; I think this looks nice.

DrahtBot removed the label CI failed on Nov 14, 2023

luke-jr changes_requested

luke-jr commented at 11:48 pm on November 15, 2023: member

Needs a lot of UX work. Definitely doesn’t belong on the Help menu…

hebasto renamed this:
~~gui: getrawtransaction implementation~~
getrawtransaction implementation
on Nov 20, 2023

in src/qt/utilitydialog.cpp:72 in e233da29db outdated

124+            ui->txidEdit->setVisible(false);
125+            ui->labelBlockHash->setVisible(false);
126+            ui->blockHashEdit->setVisible(false);
127+            ui->submitButton->setVisible(false);
128+            ui->verboseCheckbox->setVisible(false);
129+            break;

pablomartin4btc commented at 3:57 pm on November 22, 2023:

Since you are here (refactoring commit 10b3cd129e1fa8ece229a255d6fff89f419751c0) I’d extract all these settings into a function like “initializeAboutModeWindow” or something like that, same for the other modes, making the code a bit more maintainable and easier to follow.

pablomartin4btc commented at 3:58 pm on November 22, 2023: contributor

Concept ACK

As @luke-jr perhaps we can create a separate menu item like “Tools” or “Utils” with this feature in it (and would be more suitable if we need to add for example other features as @willcl-ark mentioned).

Light CR.

BrandonOdiwuor force-pushed on Dec 20, 2023

Gui: getrawtransaction Implementation

Add getrawtransaction RPC to GUI to tools > getrawtransactions

9659890f7c

BrandonOdiwuor force-pushed on Dec 20, 2023

DrahtBot added the label CI failed on Dec 20, 2023

DrahtBot removed the label CI failed on Dec 20, 2023

hernanmarino approved

hernanmarino commented at 6:36 pm on December 20, 2023: contributor

Concept ACK.

BrandonOdiwuor commented at 3:59 am on December 21, 2023: contributor

I have updated the PR based on the feedback given above:

Added Tools menu
Moved getrawtransaction RPC implementation to Tools > verify external txid

cc: @willcl-ark @luke-jr @pablomartin4btc

in src/qt/forms/toolsdialog.ui:24 in 9659890f7c

19+        <number>12</number>
20+       </property>
21+       <item row="0" column="0">
22+        <widget class="QLabel" name="labelTxId">
23+         <property name="text">
24+          <string>transaction id: </string>

pablomartin4btc commented at 2:07 pm on December 28, 2023:

nit:

0          <string>Transaction id: </string>

or ID?

in src/qt/forms/toolsdialog.ui:54 in 9659890f7c

49+        </widget>
50+       </item>
51+       <item row="2" column="0">
52+        <widget class="QLabel" name="labelBlockHash">
53+         <property name="text">
54+          <string>blockhash (optional): </string>

pablomartin4btc commented at 2:08 pm on December 28, 2023:

nit:

0          <string>Blockhash (optional): </string>

in src/qt/bitcoingui.cpp:372 in 9659890f7c

368@@ -368,12 +369,17 @@ void BitcoinGUI::createActions()
369     m_mask_values_action->setStatusTip(tr("Mask the values in the Overview tab"));
370     m_mask_values_action->setCheckable(true);
371 
372+    getRawTransactionAction = new QAction(tr("&Verify external txid"), this);

pablomartin4btc commented at 2:23 pm on December 28, 2023:

nit:

0    getRawTransactionAction = new QAction(tr("&Verify transaction"), this);

Some thoughts: perhaps we use a more generic naming here and in the form we indicate if it’s a wallet transaction with a checkbox (and then we change the form removing the blockhash option and call gettransaction?). Or maybe just on a follow-up.

pablomartin4btc commented at 2:24 pm on December 28, 2023: contributor

utACK 9659890f7c9223d85dda3d066c2e3de42cabe7cf

I agree that the new place for this PR’s feature looks better under the new “Tools” menu.

Also code-wise it’s better to have a new separate dialog (toolsdialog) rather than re-using the existing help dialog (HelpMessageDialog). Thinking it loud, I wonder if we need to specify a more specific name instead of the current generic one (what other use cases would open this ToolsDialog? Other RPC commands?). Another thoughts: how this new feature (and future ones?) integrates with the current RPC console? Should we also have some kind of link on the form to open the console with the execution of the rpc command and its output? Should we create a new sub-menu under “Tools” like “RPC commands”? These are some questions that don’t need to be answered right now nor stopping this PR to go ahead and get merged, just to keep the focus on what directions we want to give to the UI and follow it up.

DrahtBot requested review from hernanmarino on Dec 28, 2023

DrahtBot removed review request from hernanmarino on Dec 28, 2023

DrahtBot requested review from hernanmarino on Dec 28, 2023

kristapsk commented at 4:46 pm on December 28, 2023: contributor

Concept ACK

DrahtBot removed review request from hernanmarino on Dec 28, 2023

DrahtBot requested review from hernanmarino on Dec 28, 2023

DrahtBot added the label CI failed on Jan 2, 2024

DrahtBot removed the label CI failed on Jan 4, 2024

alfonsoromanz approved

alfonsoromanz commented at 8:28 pm on January 9, 2024: contributor

Tested ACK https://github.com/bitcoin-core/gui/pull/777/commits/9659890f7c9223d85dda3d066c2e3de42cabe7cf

screen-recording

DrahtBot removed review request from hernanmarino on Jan 9, 2024

DrahtBot requested review from hernanmarino on Jan 9, 2024

DrahtBot added the label CI failed on Jan 14, 2024

hebasto commented at 1:36 pm on February 12, 2024: member

Concept ACK. However, I still agree with @promag’s https://github.com/bitcoin/bitcoin/issues/19161#issuecomment-667726216:

GUI users can use the RPC console.

DrahtBot removed review request from hernanmarino on Feb 12, 2024

DrahtBot requested review from hernanmarino on Feb 12, 2024

maflcko commented at 8:42 am on February 13, 2024: contributor

Seems confusing to have the same feature present twice. Once in this pop-up and another time in the RPC pop-up?

If this is merged, then every RPC will be duplicated in a new GUI pop-up?

DrahtBot removed review request from hernanmarino on Feb 13, 2024

DrahtBot requested review from hernanmarino on Feb 13, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

DrahtBot commented at 2:13 pm on August 28, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot commented at 1:01 am on November 25, 2024: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

getrawtransaction implementation #777

Reviews

Conflicts