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

dougEfresh commented at 7:14 PM on January 25, 2023: contributor

Remove optional rpc doc for getrawtransaction when verbose is 2

DrahtBot commented at 7:14 PM on January 25, 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	stickies-v

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

DrahtBot added the label Docs on Jan 25, 2023

in src/rpc/rawtransaction.cpp:216 in 8fc0d08698 outdated

 209 | @@ -210,13 +210,13 @@ static RPCHelpMan getrawtransaction()
 210 |                          RPCResult::Type::OBJ, "", "",
 211 |                          {
 212 |                              {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
 213 | -                            {RPCResult::Type::NUM, "fee", /*optional=*/true, "transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"},
 214 | +                            {RPCResult::Type::NUM, "fee", "Transaction fee in " + CURRENCY_UNIT},

stickies-v commented at 11:45 AM on January 26, 2023:

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

dougEfresh commented at 11:53 AM on January 31, 2023:

The entire block is included only if undo data is present

stickies-v commented at 11:57 AM on January 31, 2023:

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

stickies-v commented at 2:11 PM on March 3, 2023:

@dougEfresh this seems unaddressed?

fanquake requested review from willcl-ark on Jan 26, 2023

in src/rpc/rawtransaction.cpp:219 in 8fc0d08698 outdated

 217 | -                                {RPCResult::Type::OBJ, "", "utxo being spent, omitted if block undo data is not available",
 218 | +                                {RPCResult::Type::OBJ, "", "utxo being spent",
 219 |                                  {
 220 |                                      {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
 221 | -                                    {RPCResult::Type::OBJ, "prevout", /*optional=*/true, "Only if undo information is available)",
 222 | +                                    {RPCResult::Type::OBJ, "prevout", "The previous transaction",

stickies-v commented at 11:49 AM on January 26, 2023:

"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.

stickies-v commented at 2:11 PM on March 3, 2023:

@dougEfresh this seems unaddressed?

stickies-v changes_requested

willcl-ark commented at 1:51 PM on January 26, 2023: contributor

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 commented at 2:08 PM on January 26, 2023: contributor

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 commented at 11:35 AM on January 30, 2023: contributor

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 commented at 10:35 AM on February 16, 2023: member

@dougEfresh are you going to followup here?

maflcko commented at 5:54 PM on March 2, 2023: member

up for grabs?

dougEfresh commented at 6:05 PM on March 2, 2023: contributor

up for grabs? @MarcoFalke on it. Just been super busy last few weeks.

dougEfresh force-pushed on Mar 3, 2023

dougEfresh commented at 1:27 PM on March 3, 2023: contributor

@stickies-v This better?

dougEfresh force-pushed on Mar 3, 2023

stickies-v commented at 9:18 PM on March 4, 2023: contributor

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 https://github.com/stickies-v/bitcoin/commit/3280c03cc1dc412f2719de0ecf926eb1d70a0679 - feel free to use.

dougEfresh force-pushed on Mar 6, 2023

dougEfresh commented at 6:14 PM on March 6, 2023: contributor

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

stickies-v commented at 7:03 PM on March 6, 2023: contributor

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

<details> <summary>git diff 3280c03 3ecb9458c05ac869bc2d77ea983bc85baf9baa07 -- src/rpc/rawtransaction.cpp</summary>

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"},

</summary>

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

dougEfresh force-pushed on Mar 6, 2023

stickies-v approved

stickies-v commented at 9:54 PM on March 7, 2023: contributor

ACK 3e947d7117c97a3cc34cfa7e1f5515fa0192fbe7

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.

fanquake merged this on Mar 8, 2023

fanquake closed this on Mar 8, 2023

dougEfresh commented at 3:31 PM on March 8, 2023: contributor

@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.

sidhujag referenced this in commit a6f9afbfa3 on Mar 8, 2023

bitcoin locked this on Mar 7, 2024