doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2 #26968

dougEfresh · 2023-01-25T19:14:19Z

Remove optional rpc doc for getrawtransaction when verbose is 2

DrahtBot · 2023-01-25T19:14:22Z

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	stickies-v

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

stickies-v · 2023-01-26T11:45:03Z

src/rpc/rawtransaction.cpp

-                            {RPCResult::Type::NUM, "fee", /*optional=*/true, "transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"},
+                            {RPCResult::Type::NUM, "fee", "Transaction fee in " + CURRENCY_UNIT},


The current wording and optionality seems to be correct to me, I think the fee is only included if undo data is present?

The entire block is included only if undo data is present

"fee" is not part of the "prevout" object, it is its own, top-level KV pair

@dougEfresh this seems unaddressed?

stickies-v · 2023-01-26T11:49:07Z

src/rpc/rawtransaction.cpp

                                {
                                    {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
-                                    {RPCResult::Type::OBJ, "prevout", /*optional=*/true, "Only if undo information is available)",
+                                    {RPCResult::Type::OBJ, "prevout", "The previous transaction",


"prevout" refers to output, not a transaction. Also here, /*optional=*/true and Only if undo information is available seem correct to me, I'm open to adding a description of prevout being the previous output.

@dougEfresh this seems unaddressed?

willcl-ark · 2023-01-26T13:51:00Z

I agree with @stickies-v's comments here. We are not going to be returning those fields unless the undo data is available (for the previous coin), so I think the wording is correct as-is?

stickies-v · 2023-01-26T14:08:58Z

so I think the wording is correct as-is?

Not sure if you're implying a NACK with this, but this wording (i.e. the one initially highlighted by MarcoFalke) is incorrect imo and should indeed be fixed by this PR. vin is also returned if no undo data, it just won't contain a prevout field.

Concept ACK (so our beloved bot doesn't misinterpret me (edit: ugh, didn't work))

willcl-ark · 2023-01-30T11:35:03Z

so I think the wording is correct as-is?

Not sure if you're implying a NACK with this, but this wording (i.e. the one initially highlighted by MarcoFalke) is incorrect imo and should indeed be fixed by this PR. vin is also returned if no undo data, it just won't contain a prevout field.

Right. Sorry I wasn't clear; I'm agreeing with you on both counts: 1 wording item to be changed (L216), 2 wording items correct as they are (L213 & L219).

fanquake · 2023-02-16T10:35:26Z

@dougEfresh are you going to followup here?

maflcko · 2023-03-02T17:54:45Z

up for grabs?

dougEfresh · 2023-03-02T18:05:49Z

up for grabs?

@MarcoFalke on it. Just been super busy last few weeks.

dougEfresh · 2023-03-03T13:27:09Z

@stickies-v This better?

stickies-v · 2023-03-04T21:18:26Z

I've provided links [1, 2] in my previous comments to reference the code from which I've built my understanding about which fields are optional and which ones aren't. Are we disagreeing about behaviour? My understanding in short is: "vin" is always present for verbosity>=1, whereas "fee" and "prevout" are omitted if no block data.

So to me it seems like the latest force push (0404143) removes the only change that actually had to be made and what's left is incorrect.

I've committed the changes I think need to be made to stickies-v@3280c03 - feel free to use.

dougEfresh · 2023-03-06T18:14:37Z

@stickies-v I agree with your change, have pushed it to this PR.
Thanks

stickies-v · 2023-03-06T19:03:52Z

I think ci is failing because you didn't fully apply my commit:

git diff 3280c03 3ecb945 -- src/rpc/rawtransaction.cpp

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 21d49fda9..986e7c0cc 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -219,7 +219,7 @@ static RPCHelpMan getrawtransaction()
                                 {RPCResult::Type::OBJ, "", "utxo being spent",
                                 {
                                     {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
-                                    {RPCResult::Type::OBJ, "prevout", /*optional=*/true, "The previous output, omitted if block undo data is not available",
+                                    {RPCResult::Type::OBJ, /*optional=*/true, "The previous output, omitted if block undo data is not available",
                                     {
                                         {RPCResult::Type::BOOL, "generated", "Coinbase or not"},
                                         {RPCResult::Type::NUM, "height", "The height of the prevout"},

stickies-v

ACK 3e947d7

Note: I'm glad that this is now in good shape, but for full transparency: to me this was a very frustrating review process and even though I hold no personal grudges over it, this will likely affect my enthusiasm towards reviewing your future work and for that reason I want to be transparent and share my feedback with you. I spent a lot of time writing out my thoughts and reasoning with links etc across multiple reviews, and yet every additional force-push felt like a "throw something at the wall and see if it sticks" approach. I have no problem whatsoever with clarifying misunderstanding or disagreement, but that was not the case - my comments were largely just ignored/unanswered. For such a trivial change, I spent way too much review time hand-holding.

dougEfresh · 2023-03-08T15:31:16Z

@stickies-v I felt the same as you. The time spent on such a trivial change was too much. My apologies for not being more thorough with communication when I had questions. I assumed too much without 1st getting clarification. There too many threads and thus communication was lost. Again, my apologies for not being more communicative.

…wtransaction when verbose is 2 3e947d7 doc: remove 'omitted...' doc for rpc getrawtransaction when verbose is 2 (dougEfish) Pull request description: Remove optional rpc doc for getrawtransaction when verbose is 2 ACKs for top commit: stickies-v: ACK 3e947d7 Tree-SHA512: b9e970d6ef4a47ec7ca32f5ff1028cc901f1bfdc1571668208505d42f4160733530601b78e469de82a854d3b298a55a81d0a7916bc5db4a43ad6d6a299c55c9e

DrahtBot added the Docs label Jan 25, 2023

dougEfresh mentioned this pull request Jan 25, 2023

doc: Fixup getrawtransaction RPC docs #26734

Merged

fanquake requested a review from willcl-ark January 26, 2023 11:48

stickies-v suggested changes Jan 26, 2023

View reviewed changes

dougEfresh force-pushed the doc-rpcrawtransaction-verbose2 branch from 8fc0d08 to 47f1292 Compare March 3, 2023 13:26

dougEfresh force-pushed the doc-rpcrawtransaction-verbose2 branch from 47f1292 to 0404143 Compare March 3, 2023 19:41

dougEfresh force-pushed the doc-rpcrawtransaction-verbose2 branch from 0404143 to 3ecb945 Compare March 6, 2023 18:12

doc: remove 'omitted...' doc for rpc getrawtransaction when verbose is 2

3e947d7

dougEfresh force-pushed the doc-rpcrawtransaction-verbose2 branch from 3ecb945 to 3e947d7 Compare March 6, 2023 19:53

stickies-v approved these changes Mar 7, 2023

View reviewed changes

fanquake merged commit 8d12127 into bitcoin:master Mar 8, 2023

bitcoin locked and limited conversation to collaborators Mar 7, 2024

		{RPCResult::Type::NUM, "fee", /optional=/true, "transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"},
		{RPCResult::Type::NUM, "fee", "Transaction fee in " + CURRENCY_UNIT},

doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2 #26968

doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2 #26968

Uh oh!

Conversation

dougEfresh commented Jan 25, 2023

Uh oh!

DrahtBot commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

stickies-v Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

dougEfresh Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

stickies-v Mar 3, 2023

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Jan 26, 2023

Uh oh!

stickies-v commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willcl-ark commented Jan 30, 2023

Uh oh!

fanquake commented Feb 16, 2023

Uh oh!

maflcko commented Mar 2, 2023

Uh oh!

dougEfresh commented Mar 2, 2023

Uh oh!

dougEfresh commented Mar 3, 2023

Uh oh!

stickies-v commented Mar 4, 2023

Uh oh!

dougEfresh commented Mar 6, 2023

Uh oh!

stickies-v commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

dougEfresh commented Mar 8, 2023

Uh oh!

Uh oh!

DrahtBot commented Jan 25, 2023 •

edited

Loading

stickies-v Jan 31, 2023 •

edited

Loading

stickies-v commented Jan 26, 2023 •

edited

Loading

stickies-v commented Mar 6, 2023 •

edited

Loading