Remove taproot chain param #24737

maflcko · 2022-04-01T12:26:33Z

Now that the commit (7c08d81) which changes taproot to be enforced for all blocks is sufficiently buried by other commits, and thus less likely to be reverted, it seems a good time to remove no longer needed non-consensus code.

In particular, the taproot deployment status will be reported on RPC. However, the result is neither wrong nor correct. While it can tell the block height at which taproot activated, it doesn't reflect that the rules are enforced since genesis. Since it is possible to determine the taproot activation height by running a previous release or simply looking it up in the BIP, there is no need to report it in a field.

test/functional/feature_taproot.py

DrahtBot · 2022-04-01T12:58:32Z

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

Conflicts

No conflicts as of last run.

Sjors · 2022-04-05T21:06:27Z

test/functional/rpc_blockchain.py

@@ -213,19 +213,6 @@ def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash,
                },
                'active': False
            },
-            'taproot': {


The other soft forks are marked as burried, rather than completely omitted.

Yes, I think this is a reason to remove them as well. Once they are buried, their activation height is a constant. So there is no need to report it dynamically over RPC. Apart from being overkill, I don't see a use case either.

If someone wants to know if the wallet supports $feature, they can use a wallet RPC or the GUI. (This RPC won't help them)
If someone wants to know if the mempool supports $feature, this RPC won't help them either. I think the best they can do is read the release notes to figure out if the mempool supports $feature regardless of blockchain height. Alternatively they can just test if their use case is supported.
If someone read the release notes, but wants to know over RPC which version of Bitcoin Core they are running, they can use the appropriate call.

As mentioned in OP, returning a result for taproot here (buried or vb) is neither right or wrong. While the result may reflect the height at which the BIP activated on the network (as mentioned in the BIP as well), it does not reflect the height at which this software enforces the rules. The rules are theoretically enforced since genesis, practically only after witness is allowed in blocks. This is not something that can/should be explained by returning a JSON.

If you insist on returning a JSON, my preference would be to return 'taproot': True. Until then, my preference is to remove it, since it is unknown if there is a use case for this (and if there is one) what the use case is.

Yes, I think this is a reason to remove them as well.

I'm not opposed to that, but the inconsistency doesn't help review. Why not set it to burried now and make another PR to drop all buried soft forks from the RPC, using the above reasoning. Such a followup would be an RPC-only change.

Ok, my suggestion to remove the others as well was a bit over eager. They are still used and the activation height actually affects code paths. In fact the tests use -testactivationheight to mock the activation height. Though, this isn't true for taproot.

Also, I already tried to bury taproot in #23505. Though there wasn't much/enough appetite to get it in before the 23.0 feature freeze. As burying is a breaking RPC change and we already had one (getdeploymentinfo) in 23.0, it could have been bundled with that. However, now that 23.0 is out, it would be a breaking change in 24.0 to bury it. Then, if it is decided to remove, it will be another breaking change.

Why is it a breaking change? A correct parser should look for taproot and then check the type field. It should be able to handle buried. If an implementation can't handle that, it might also break when taproot is missing entirely.

ajtowns · 2022-09-07T17:28:23Z

Since it is possible to determine the taproot activation height by running a previous release or simply looking it up in the BIP, there is no need to report it in a field.

I don't think I quite agree with this -- it seems better to me to have the program document its own behaviour, rather than expecting you to look up documentation that might not even be in sync with the code (our behaviour since #23536 isn't documented in a BIP, is it?). So, better to keep the report, but have it match what the code is doing. For example, it might be better to report something like:

$ bitcoin-cli getdeploymentinfo 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad | jq .deployments.taproot
{
  "type": "almost_always",
  "active": true,
  "almost_always": {
      "active_this_block": false,
      "exception_blocks": [
          "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
          "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad",
      ]
  },
}

(and perhaps likewise for bip16/p2sh, though going overboard with reporting ancient changes that no block violates doesn't seem particularly useful).

Sjors · 2022-09-09T11:17:11Z

I like the exception block listing. Maybe a little less verbose:

{
  "type": "buried",
  "active": true,
  "exception_blocks": [
    "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
    "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad",
  ]
}

Here active refers to the block you're requesting and buried implies the existence of exception_blocks, which may be empty.

ajtowns · 2022-09-09T11:46:33Z

Here active refers to the block you're requesting

active refers to the following block (and mempool, if that block is the current tip):

      "active" : true|false,            (boolean) true if the rules are enforced for the mempool and the next block

Should have a consistent meaning across deployment types, though I could see doing a breaking change for all deployment types to make it less confusing...

and buried implies the existence of exception_blocks, which may be empty.

Elsewhere, we put any extra info that's specific to a deployment method in a sub-object, named after the type (ie "bip9": {...}). Maybe { "buried": { "exceptions": [ ... ] } } might be reasonable?

Sjors · 2022-09-09T11:49:50Z

Ah ok, then active_this_block makes sense. But we can also drop it, because the user can check exceptions.

"buried": { "exceptions": could make sense yes.

maflcko · 2022-09-12T07:59:42Z

it seems better to me to have the program document its own behaviour, rather than expecting you to look up documentation that might not even be in sync with the code (our behaviour since #23536 isn't documented in a BIP, is it?)

It is documented in doc/bips.md as part of this pull.

though going overboard with reporting ancient changes that no block violates doesn't seem particularly useful

Agree. I've added a second commit, though let me know if I should drop it again.

Sjors

I like the new exception blocks field. I still prefer to mark taproot as buried rather than dropping it.

src/rpc/blockchain.cpp

Sjors · 2022-09-12T08:23:21Z

test/functional/rpc_blockchain.py

@@ -213,19 +213,6 @@ def check_signalling_deploymentinfo_result(self, gdi_result, height, blockhash,
                },
                'active': False
            },
-            'taproot': {


Why is it a breaking change? A correct parser should look for taproot and then check the type field. It should be able to handle buried. If an implementation can't handle that, it might also break when taproot is missing entirely.

ajtowns · 2022-09-12T12:58:45Z

Don't really like the idea of reporting the exceptions without reporting what's "normal".

I was thinking more along the lines of: ajtowns@809a545 (possibly reporting bip16 support/exceptions as well) which gives output like:

{
  "hash": "00000000000000000007e4449a4d61ba2d2a3f0bbc0bbf355d2fcc8deecdb864",
  "height": 753789,
  "deployments": {
    ...
    "bip16": {
      "type": "buried",
      "buried": {
        "exceptions": [
          "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"
        ]
      },
      "active": true,
      "height": 0
    },
    ...
    "segwit": {
      "type": "buried",
      "active": true,
      "height": 481824
    },
    "taproot": {
      "type": "buried",
      "buried": {
        "exceptions": [
          "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
          "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad"
        ]
      },
      "active": true,
      "height": 0
    }
  }
}

maflcko · 2022-09-13T13:31:30Z

Ok, leaving up for grabs for now

Sjors · 2022-09-23T16:37:14Z

Picked this up along with @ajtowns's changes in #26162. Work in progress though.

maflcko added the RPC/REST/ZMQ label Apr 1, 2022

maflcko mentioned this pull request Apr 1, 2022

Enforce Taproot script flags whenever WITNESS is set #23536

Merged

jonatack reviewed Apr 1, 2022

View reviewed changes

test/functional/feature_taproot.py Outdated Show resolved Hide resolved

maflcko force-pushed the 2204-rem-code- branch 2 times, most recently from faea171 to fa47617 Compare April 1, 2022 12:40

maflcko force-pushed the 2204-rem-code- branch from fa47617 to fa8abbe Compare April 1, 2022 13:26

This was referenced Apr 1, 2022

deploymentstatus: move g_versionbitscache global to ChainstateManager #24595

Merged

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Closed

maflcko mentioned this pull request Apr 5, 2022

test: use MiniWallet for mempool_unbroadcast.py #24749

Merged

Sjors reviewed Apr 5, 2022

View reviewed changes

maflcko force-pushed the 2204-rem-code- branch from fa8abbe to 88e1aa6 Compare April 21, 2022 12:55

maflcko force-pushed the 2204-rem-code- branch 2 times, most recently from 28cb18e to fa1cf13 Compare April 28, 2022 18:54

DrahtBot added the Needs rebase label May 13, 2022

maflcko force-pushed the 2204-rem-code- branch from fa1cf13 to fae6864 Compare May 13, 2022 11:15

DrahtBot removed the Needs rebase label May 13, 2022

This was referenced Aug 29, 2022

Update chainparams for 24.0 release #25946

Merged

Add headerssync tuning parameters optimization script to repo #25970

Merged

DrahtBot added the Needs rebase label Sep 6, 2022

maflcko force-pushed the 2204-rem-code- branch from fae6864 to faf3b41 Compare September 12, 2022 07:59

Remove taproot chain param

2222ea8

Sjors reviewed Sep 12, 2022

View reviewed changes

maflcko force-pushed the 2204-rem-code- branch from faf3b41 to fa3d09a Compare September 12, 2022 08:36

DrahtBot removed the Needs rebase label Sep 12, 2022

rpc: Report script_flag_exceptions in getdeploymentinfo

fa7b5ca

maflcko force-pushed the 2204-rem-code- branch from fa3d09a to fa7b5ca Compare September 12, 2022 09:03

maflcko closed this Sep 13, 2022

maflcko deleted the 2204-rem-code-🥊 branch September 13, 2022 13:31

vasild mentioned this pull request Sep 16, 2022

Segmentation fault in the scheduler thread when an index fails to commit to the db #26068

Closed

Sjors mentioned this pull request Sep 23, 2022

Remove taproot chain param and list exception blocks in getdeploymentinfo #26162

Closed

Sjors mentioned this pull request Sep 29, 2022

Remove Taproot activation height #26201

Open

bitcoin locked and limited conversation to collaborators Sep 23, 2023

Remove taproot chain param #24737

Remove taproot chain param #24737

Uh oh!

Conversation

maflcko commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 6, 2022 • 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!

Sjors Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Sep 7, 2022

Uh oh!

Sjors commented Sep 9, 2022

Uh oh!

ajtowns commented Sep 9, 2022

Uh oh!

Sjors commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 12, 2022

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Sep 12, 2022

Uh oh!

maflcko commented Sep 13, 2022

Uh oh!

Sjors commented Sep 23, 2022

Uh oh!

Uh oh!

maflcko commented Apr 1, 2022 •

edited

Loading

DrahtBot commented Apr 1, 2022 •

edited

Loading

maflcko Apr 6, 2022 •

edited

Loading

Sjors commented Sep 9, 2022 •

edited

Loading