longpollid and data params to template_request
#28056
This PR will add the optional parameters longpollid and data to template_request as they were missing when calling help getblocktemplate in RPCHelpMan.
I request the maintainers to review this and let me know about any mistakes in the descriptions of the parameters.
This PR refers to the issue #27998
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, russeree |
| Concept ACK | luke-jr |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
This doesn’t compile. Ideally, you compile and test any changes locally before opening a pull request.
I see, I’ll try to compile it on my end and will let you know if there’s any issue. Also, can you please check the description for the new parameters added if they need any changes?
565@@ -566,6 +566,8 @@ static RPCHelpMan getblocktemplate()
566 {"segwit", RPCArg::Type::STR, RPCArg::Optional::NO, "(literal) indicates client side segwit support"},
567 {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "other client side supported softfork deployment"},
568 }},
569+ {"longpollid", RPCArg::Type::STR, RPCArg::Optional::OMITTED, " "longpollid" of job to monitor for expiration; required and valid only for long poll requests"},
0 {"longpollid", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Delay processing request until the result would vary significantly from the \"longpollid\" of a prior template."},
It doesn’t necessarily imply the previous template is expired.
565@@ -566,6 +566,8 @@ static RPCHelpMan getblocktemplate()
566 {"segwit", RPCArg::Type::STR, RPCArg::Optional::NO, "(literal) indicates client side segwit support"},
567 {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "other client side supported softfork deployment"},
568 }},
569+ {"longpollid", RPCArg::Type::STR, RPCArg::Optional::OMITTED, " "longpollid" of job to monitor for expiration; required and valid only for long poll requests"},
570+ {"data", RPCResult::Type::STR_HEX, RPCArg::Optional::OMITTED, "Block data encoded in hexadecimal"},
0 {"data", RPCResult::Type::STR_HEX, RPCArg::Optional::OMITTED, "Proposed block data to check, encoded in hexadecimal; valid only for mode=\"proposal\""},
This doesn’t compile. Ideally, you compile and test any changes locally before opening a pull request.
Hey so I was trying to build it after adding the changes you suggested, but i’m getting this error in VS, as I’m building on Windows 11. Can you please help what this means?
I followed this guide and opened bitcoin.sln in VS.
I strongly recommend not building in Windows/MSVC, as it’s a venture on its own to get that to work. In a Linux VM, or WSL2, it should be fairly straightforward.
Ohh I see! Then I’ll install WSL2 and try building it from there. I suppose this guide should be enough to get it working?
mining.cpp. Should I revert this commit, or something else?
Don’t revert the commit; just remove the stray file and squash.
I think it should be fine now, can you please check once again?
You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
I think I did squash them, atleast that’s what it shows in the commits tab.
vcpkg” now, though.
Apologies, you did indeed!
No worries, thanks for the guidance throughout this PR!
Apologies, you did indeed! The commit title is “Deleted stray file
vcpkg” now, though.
Should I amend this commit or leave it as it is? I’m not sure if I can change the commit name now.
565@@ -566,6 +566,8 @@ static RPCHelpMan getblocktemplate()
566 {"segwit", RPCArg::Type::STR, RPCArg::Optional::NO, "(literal) indicates client side segwit support"},
567 {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "other client side supported softfork deployment"},
568 }},
569+ {"longpollid", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Delay processing request until the result would vary significantly from the \"longpollid\" of a prior template."},
570+ {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "Proposed block data to check, encoded in hexadecimal; valid only for mode=\"proposal\""},
565@@ -566,6 +566,8 @@ static RPCHelpMan getblocktemplate()
566 {"segwit", RPCArg::Type::STR, RPCArg::Optional::NO, "(literal) indicates client side segwit support"},
567 {"str", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "other client side supported softfork deployment"},
568 }},
569+ {"longpollid", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "Delay processing request until the result would vary significantly from the \"longpollid\" of a prior template."},
No problem, this is what I got running your code.
WIP just means “this is not ready for review yet”.
You’re always expected to address review comments, whether that means making changes, or giving a justification why it’s better not to.
Bitcoin development is always bottlenecked and all changes no matter how small must thoroughly reviewed for and allowed time for social consensus since every change could potentially cost people real world value. You can work on other issues and/or await more feedback.
Ahh I see! Thanks for clearing it up :D I was just worried if I needed to change something from my side hehe
Thanks for all the help in this PR everyone, really learnt a great deal from each one of you!
Added `longpollid` and `data` params to `template_request` in `getblocktemplate` #27998
tACK f6a26196cfb7e2c90e25f82b0e2f569a05013cae
Thanks for picking this up and swiftly addressing feedback!
Concept ACK
I do have a few questions about the implementation of the RPCHelpMan parameter text for ’longpollid’ when used for a template instead of a propsal. My primary issue is the block proposal definition for longpollid at https://github.com/bitcoin/bitcoin/blob/57b8336dfed6312003cf34cd5ae7099f77115e73/src/rpc/mining.cpp#L613
varies from the current PRs when used as an optional parameter for a block template. https://github.com/bitcoin/bitcoin/blob/f6a26196cfb7e2c90e25f82b0e2f569a05013cae/src/rpc/mining.cpp#L569 But the use of the passed parameter for longpollid at https://github.com/bitcoin/bitcoin/blob/57b8336dfed6312003cf34cd5ae7099f77115e73/src/rpc/mining.cpp#L715-L721 is the same for both a template and a proposal.
The questions I have are
Should the RPCHelpMan text for longpollid match for proposals and templates or does the behavior vary based on if the user is passing longpollid for a proposal vs template? My answer is going to be I assume that the behavior is different but I can’t seem to find the logic where it differs.
Should there be a mention of the 60 second timeout if we are going to describe not just the id component of the longpollid but the behavior of the optional parameter?
I apologize if I missed something completely.
But the use of the passed parameter for
longpollidat https://github.com/bitcoin/bitcoin/blob/57b8336dfed6312003cf34cd5ae7099f77115e73/src/rpc/mining.cpp#L715-L721is the same for both a template and a proposal.
longpollid is neither used as a parameter nor returned as a result for mode==proposal. The line you reference is never reached in that case, as we return or throw within this code block. See also the corresponding results.
- Should the RPCHelpMan text for
longpollidmatch for proposals and templates or does the behavior vary based on if the user is passinglongpollidfor a proposal vs template?
As before, it’s not used for proposal mode. Could mention that in the description but I don’t think it’s that relevant, conceptually longpolling on a proposal doesn’t make sense I think.
- Should there be a mention of the 60 second timeout if we are going to describe not just the id component of the longpollid but the behavior of the optional parameter?
It only “times out” if there are new transactions to be considered as per this break, so I think the current description works well. Moreover, we may want to change this implementation detail (it’s not part of the BIP22 spec), so keeping it out of the documentation makes that an easier process.
is the same for both a template and a proposal.
longpollidis neither used as a parameter nor returned as a result formode==proposal. The line you reference is never reached in that case, as we return or throw within this code block. See also the corresponding results.
- Should the RPCHelpMan text for
longpollidmatch for proposals and templates or does the behavior vary based on if the user is passinglongpollidfor a proposal vs template?As before, it’s not used for
proposalmode. Could mention that in the description but I don’t think it’s that relevant, conceptually longpolling on a proposal doesn’t make sense I think.
Thank you for this excellent overview, Thank you for taking the time so really dive into how this works. Sorry for being incorrect.
- Should there be a mention of the 60 second timeout if we are going to describe not just the id component of the longpollid but the behavior of the optional parameter?
It only “times out” if there are new transactions to be considered as per this
break, so I think the current description works well. Moreover, we may want to change this implementation detail (it’s not part of the BIP22 spec), so keeping it out of the documentation makes that an easier process.
Makes sense.
tACK https://github.com/bitcoin/bitcoin/commit/f6a26196cfb7e2c90e25f82b0e2f569a05013cae
Labels
RPC/REST/ZMQ
Milestone
26.0