Use the same format as `zcashd` for `getblocktemplate` proposal mode errors #5981

teor2345 · 2023-01-17T07:30:35Z

Motivation

When I use zebra-utils/zcash-rpc-block-template-to-proposal from PR #5944 to send a block proposal from Zebra to both zcashd and Zebra, I see these differences in the response format:

Proposal response diffs between ports  8232 6666 and time sources CurTime MinTime MaxTime ClampedNow:
--- /tmp/tmp.UWcAoSvcUZ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 15:15:37.881314992 -0300
+++ /tmp/tmp.UWcAoSvcUZ.block-template-proposal/proposal-check-getblocktemplate-proposal.MaxTime.6666.json	2023-01-26 15:15:38.329290891 -0300
@@ -0,0 +1,5 @@
+invalid proposal: Block {
+    source: ValidateProposal(
+        "proposal is not based on the current best chain tip: previous block hash must be the best chain tip",
+    ),
+}

Zebra should return a string to match the zcashd response format, but it is currently returning JSON. For example zcashd returns errors like bad-txns-inputs-missingorspent

Specifications

We were following the specifications in the bitcoin improvement proposals here, but being compatible with the zcashd implementation is a higher priority.

It is enough to return any error string in kebab-case format - we don't need to match the exact error wording.

For the example above about best tip error zebra should return something like:

invalid-proposal-proposal-is-not-based-on-the-current-best-chain-tip-previous-block-hash–must–be-the–best-chain-tip

Complex Code or Requirements

This should be a simple format change.

Testing

We can use the zebra-utils/zcash-rpc-block-template-to-proposal script to manually re-test to make sure this bug is gone. We'll need to wait for an error.

(The previous block error happens if a new block has just arrived, or there is a chain fork.)

The text was updated successfully, but these errors were encountered:

teor2345 · 2023-01-17T08:06:06Z

@mpguerra we might want to schedule this fix in this sprint, the different outputs could make testing harder for everyone, because we have to check the differences every time we run the script. But this is not a consensus rule bug, so it's not as important as #5982.

@arya2 what do you think?

arya2 · 2023-01-17T19:26:04Z

@mpguerra we might want to schedule this fix in this sprint, the different outputs could make testing harder for everyone, because we have to check the differences every time we run the script. But this is not a consensus rule bug, so it's not as important as #5982.

@arya2 what do you think?

I think it's low-priority since it only happens for rejections where the response is going to be different anyways because Zebra doesn't provide any specific reject-reasons. The intent in this case is to test the template response rather than getblocktemplate in proposal mode so I think should prioritize fixing the causes of rejections over this.

teor2345 · 2023-01-17T22:57:55Z

@mpguerra we might want to schedule this fix in this sprint, the different outputs could make testing harder for everyone, because we have to check the differences every time we run the script. But this is not a consensus rule bug, so it's not as important as #5982.
@arya2 what do you think?

I think it's low-priority since it only happens for rejections where the response is going to be different anyways because Zebra doesn't provide any specific reject-reasons. The intent in this case is to test the template response rather than getblocktemplate in proposal mode so I think should prioritize fixing the causes of rejections over this.

It could be a blocker for running with nodes if they exit or permanently fail when they get an error response they are not expecting.

So I think we might want to fix it before we ask mining pools to test Zebra.

(We expect to return a "wrong tip block" to about 1% of proposals, due to changing chain forks, and new blocks being committed while the proposal is being created.)

teor2345 · 2023-01-17T22:58:16Z

And I agree that the consensus bug is a higher priority!

mpguerra · 2023-01-20T10:44:27Z

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @dconnolly @oxarbitrage @teor2345 @upbqdn

teor2345 · 2023-01-23T22:42:51Z

We could try replacing all the punctuation and newlines in those messages with - ?

Then the message would stay detailed and readable, but it would match the format that zcashd uses.

teor2345 · 2023-01-24T19:37:14Z

@mpguerra I don't think this is a blocker for anything, except maybe for some mining pool software using Zebra.

oxarbitrage · 2023-01-26T18:59:38Z

I was not able to reproduce this issue yet. Had been using this to test: #5803 (comment)

There are failures but not the one posted here, i am unsure if this is still happening.

Other fails i was able to see were:

Zcash responding with -inconclusive-not-best-prevblk and zebra with nothing. I got this one 3 times.

Proposal response diffs between ports  8232 6666 and time sources CurTime MinTime MaxTime ClampedNow:
--- /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 14:26:53.409248928 -0300
+++ /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.6666.json	2023-01-26 14:26:53.409248928 -0300
@@ -1 +0,0 @@
-inconclusive-not-best-prevblk
--- /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 14:26:53.409248928 -0300
+++ /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.MinTime.6666.json	2023-01-26 14:26:53.425247940 -0300
@@ -1 +0,0 @@
-inconclusive-not-best-prevblk
--- /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 14:26:53.409248928 -0300
+++ /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.MaxTime.6666.json	2023-01-26 14:26:53.449246461 -0300
@@ -1 +0,0 @@
-inconclusive-not-best-prevblk
--- /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 14:26:53.409248928 -0300
+++ /tmp/tmp.xPEFRgWMHJ.block-template-proposal/proposal-check-getblocktemplate-proposal.ClampedNow.6666.json	2023-01-26 14:26:53.469245227 -0300
@@ -1 +0,0 @@
-inconclusive-not-best-prevblk

Invalid proposal in zebra but valid in zcashd. I got this one one time.

Proposal response diffs between ports  8232 6666 and time sources CurTime MinTime MaxTime ClampedNow:
--- /tmp/tmp.UWcAoSvcUZ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 15:15:37.881314992 -0300
+++ /tmp/tmp.UWcAoSvcUZ.block-template-proposal/proposal-check-getblocktemplate-proposal.MaxTime.6666.json	2023-01-26 15:15:38.329290891 -0300
@@ -0,0 +1,5 @@
+invalid proposal: Block {
+    source: ValidateProposal(
+        "proposal is not based on the current best chain tip: previous block hash must be the best chain tip",
+    ),
+}
--- /tmp/tmp.UWcAoSvcUZ.block-template-proposal/proposal-check-getblocktemplate-proposal.CurTime.8232.json	2023-01-26 15:15:37.881314992 -0300
+++ /tmp/tmp.UWcAoSvcUZ.block-template-proposal/proposal-check-getblocktemplate-proposal.ClampedNow.6666.json	2023-01-26 15:15:38.461283790 -0300
@@ -0,0 +1,5 @@
+invalid proposal: Block {
+    source: ValidateProposal(
+        "proposal is not based on the current best chain tip: previous block hash must be the best chain tip",
+    ),
+}

I am not totally sure from where the error in the original post is built, seems to be at https://github.com/ZcashFoundation/zebra/blob/main/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs#L49 ?

I can try to change that kebab-case, however not sure if we need to change kind, error or both and i am not sure if that will fix the problem either.

@teor2345 can you point me to where do you think the format needs to be changed ?

teor2345 · 2023-01-26T21:09:12Z

The exact error doesn't matter, this ticket is about the error format.

zcashd produces this format:

inconclusive-not-best-prevblk

But zebrad produces this format:

invalid proposal: Block {
    source: ValidateProposal(
        "proposal is not based on the current best chain tip: previous block hash must be the best chain tip",
    ),
}

We don't know how strict mining pools are about the response format. And if they get an error, they might just give up on using Zebra, rather than telling us.

Here is the code that needs to change:

zebra/zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs

Lines 44 to 56 in b3a4807

    
               /// Return a rejected response containing an error kind and detailed error info. 
        
               pub fn rejected<S: ToString>(kind: S, error: BoxError) -> Self { 
        
                   let kind = kind.to_string(); 
        
                   // Pretty-print the detailed error for now 
        
                   ProposalResponse::Rejected(format!("{kind}: {error:#?}")) 
        
               } 
        
               /// Return a rejected response containing just the detailed error information. 
        
               pub fn error(error: BoxError) -> Self { 
        
                   // Pretty-print the detailed error for now 
        
                   ProposalResponse::Rejected(format!("{error:#?}")) 
        
               }

Currently, zebra-rpc formats the string with punctuation, newlines, and spaces. But we want a string that looks like zcashd instead:

invalid-proposal-proposal-is-not-based-on-the-current-best-chain-tip-previous-block-hash–must–be-the–best-chain-tip

It is ok to delete or replace punctuation. It doesn't matter if the Block, source, or ValidateProposal are included or not.

oxarbitrage · 2023-01-26T22:09:20Z

I was never referring to the exact error, i already know that is not the problem. All the explication about what the miners will do (according to what you think) are not needed either.

Here what i am asking.

Your error:

-bad-txns-inputs-missingorspent
+{
+  "reject_reason": "rejected",
+  "capabilities": [
+    "proposal"
+  ]
+}

My error (zcashd produces and error but zebra does not):

-inconclusive-not-best-prevblk

In my other error (zebra produces an error and zcashd is not):

+invalid proposal: Block {
+    source: ValidateProposal(
+        "proposal is not based on the current best chain tip: previous block hash must be the best chain tip",
+    ),
+}

Making a regex or whatever to convert from

+invalid proposal: Block {
+    source: ValidateProposal(
+        "proposal is not based on the current best chain tip: previous block hash must be the best chain tip",
+    ),
+}

into invalid-proposal-proposal-is-not-based-on-the-current-best-chain-tip-previous-block-hash–must–be-the–best-chain-tip seems a bit different than doing it for

+{
+  "reject_reason": "rejected",
+  "capabilities": [
+    "proposal"
+  ]
+}

into rejected or reject-reason-rejected. I have no idea on why capabilities is there. I think we should try to remove it.

teor2345 · 2023-01-26T22:11:44Z

I was never referring to the exact error, i already know that is not the problem. All the explication about what the miners will do (according to what you think) are not needed either.

Thanks for letting me know. I explained the errors because you said you couldn't reproduce the issue.

teor2345 · 2023-01-26T22:13:05Z

I have no idea on why capabilities is there. I think we should try to remove it.

The format was already changed in PR #5993, so the error message in the ticket is out of date. Would you like to update the ticket, or did you want me to?

oxarbitrage · 2023-01-26T22:18:01Z

The format was already changed in PR #5993, so the error message in the ticket is out of date. Would you like to update the ticket, or did you want me to?

Ok, that is what i had missing. Got it now, thanks. Ill make the changes in the OP with my error.

teor2345 · 2023-01-26T22:18:35Z

Making a regex or whatever to convert

I'd suggest using str::replace():
https://doc.rust-lang.org/std/primitive.str.html#method.replace

If we do something like this:

format!("{error:?}").replace(|c| !c.is_alphanumeric(), "-").make_ascii_lowercase()

Then it doesn't matter what the debug format is, and we can change that format without breaking the RPC.

oxarbitrage · 2023-01-26T22:19:46Z

Making a regex or whatever to convert

I'd suggest using str::replace(): https://doc.rust-lang.org/std/primitive.str.html#method.replace

If we do something like this:
format!("{error:?}").replace(|c| !c.is_alphanumeric(), "-").make_ascii_lowercase()
Then it doesn't matter what the debug format is, and we can change that format without breaking the RPC.

Yes, that is what i was thinking about, not a regex really.

teor2345 mentioned this issue Jan 17, 2023

Manually test the getblocktemplate RPC creates valid blocks #5803

Closed

4 tasks

This was referenced Jan 18, 2023

Zebra returns a confusing error for block proposals when it isn't fully synced yet #5991

Closed

fix(rpc): Return detailed errors to the RPC client when a block proposal fails #5993

Merged

mpguerra added P-Low ❄️ and removed P-Medium ⚡ labels Jan 18, 2023

mpguerra moved this to 🛑 Won't Fix in Zebra Jan 19, 2023

mpguerra added this to Zebra Jan 19, 2023

mpguerra moved this from 🛑 Won't Fix to 📋 Sprint Backlog in Zebra Jan 19, 2023

oxarbitrage self-assigned this Jan 23, 2023

mpguerra removed the S-needs-triage Status: A bug report needs triage label Jan 24, 2023

oxarbitrage mentioned this issue Jan 27, 2023

fix(getblocktemplate): change error format in proposals #6044

Merged

6 tasks

mergify bot closed this as completed in #6044 Feb 2, 2023

github-project-automation bot moved this from 📋 Sprint Backlog to ✅ Done in Zebra Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same format as `zcashd` for `getblocktemplate` proposal mode errors #5981

Use the same format as `zcashd` for `getblocktemplate` proposal mode errors #5981

teor2345 commented Jan 17, 2023 •

edited by oxarbitrage

Loading

teor2345 commented Jan 17, 2023

arya2 commented Jan 17, 2023

teor2345 commented Jan 17, 2023

teor2345 commented Jan 17, 2023

mpguerra commented Jan 20, 2023

teor2345 commented Jan 23, 2023

teor2345 commented Jan 24, 2023

oxarbitrage commented Jan 26, 2023

teor2345 commented Jan 26, 2023 •

edited

Loading

oxarbitrage commented Jan 26, 2023

teor2345 commented Jan 26, 2023

teor2345 commented Jan 26, 2023

oxarbitrage commented Jan 26, 2023

teor2345 commented Jan 26, 2023 •

edited

Loading

oxarbitrage commented Jan 26, 2023 •

edited

Loading

Use the same format as zcashd for getblocktemplate proposal mode errors #5981

Use the same format as zcashd for getblocktemplate proposal mode errors #5981

Comments

teor2345 commented Jan 17, 2023 • edited by oxarbitrage Loading

Motivation

Specifications

Complex Code or Requirements

Testing

teor2345 commented Jan 17, 2023

arya2 commented Jan 17, 2023

teor2345 commented Jan 17, 2023

teor2345 commented Jan 17, 2023

mpguerra commented Jan 20, 2023

teor2345 commented Jan 23, 2023

teor2345 commented Jan 24, 2023

oxarbitrage commented Jan 26, 2023

teor2345 commented Jan 26, 2023 • edited Loading

oxarbitrage commented Jan 26, 2023

teor2345 commented Jan 26, 2023

teor2345 commented Jan 26, 2023

oxarbitrage commented Jan 26, 2023

teor2345 commented Jan 26, 2023 • edited Loading

oxarbitrage commented Jan 26, 2023 • edited Loading

Use the same format as `zcashd` for `getblocktemplate` proposal mode errors #5981

Use the same format as `zcashd` for `getblocktemplate` proposal mode errors #5981

teor2345 commented Jan 17, 2023 •

edited by oxarbitrage

Loading

teor2345 commented Jan 26, 2023 •

edited

Loading

teor2345 commented Jan 26, 2023 •

edited

Loading

oxarbitrage commented Jan 26, 2023 •

edited

Loading