Remove taproot chain param

MarcoFalke commented at 12:26 pm on April 1, 2022: member

Now that the commit (7c08d81e119570792648fe95bbacddbb1d5f9ae2) 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.

MarcoFalke added the label RPC/REST/ZMQ on Apr 1, 2022

in test/functional/feature_taproot.py:1241 in fa456ee345 outdated

1254-        self.start_nodes()
1255-        self.import_deterministic_coinbase_privkeys()
1256+#    def setup_nodes(self):
1257+#        self.add_nodes(self.num_nodes, self.extra_args         )
1258+#        self.start_nodes()
1259+#        self.import_deterministic_coinbase_privkeys()

jonatack commented at 12:30 pm on April 1, 2022:

?

MarcoFalke commented at 12:32 pm on April 1, 2022:

:sweat_smile: Thx, removed the code.

MarcoFalke force-pushed on Apr 1, 2022

DrahtBot commented at 12:58 pm on April 1, 2022: contributor

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

Conflicts

No conflicts as of last run.

MarcoFalke force-pushed on Apr 1, 2022

in test/functional/rpc_blockchain.py:217 in fa01259e25 outdated

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

Sjors commented at 9:06 pm on April 5, 2022:

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

MarcoFalke commented at 6:32 am on April 6, 2022:

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.

Sjors commented at 9:30 am on April 19, 2022:

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.

MarcoFalke commented at 9:55 am on April 19, 2022:

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.

MarcoFalke commented at 12:54 pm on April 21, 2022:

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.

Sjors commented at 8:23 am on September 12, 2022:

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.

MarcoFalke force-pushed on Apr 21, 2022

MarcoFalke force-pushed on Apr 28, 2022

DrahtBot added the label Needs rebase on May 13, 2022

MarcoFalke force-pushed on May 13, 2022

DrahtBot removed the label Needs rebase on May 13, 2022

DrahtBot added the label Needs rebase on Sep 6, 2022

ajtowns commented at 5:28 pm on September 7, 2022: contributor

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:

 0$ bitcoin-cli getdeploymentinfo 0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad | jq .deployments.taproot
 1{
 2  "type": "almost_always",
 3  "active": true,
 4  "almost_always": {
 5      "active_this_block": false,
 6      "exception_blocks": [
 7          "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
 8          "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad",
 9      ]
10  },
11}

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

Sjors commented at 11:17 am on September 9, 2022: member

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

0{
1  "type": "buried",
2  "active": true,
3  "exception_blocks": [
4    "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
5    "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad",
6  ]
7}

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

ajtowns commented at 11:46 am on September 9, 2022: contributor

Here active refers to the block you’re requesting

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

0      "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 commented at 11:49 am on September 9, 2022: member

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.

MarcoFalke force-pushed on Sep 12, 2022

MarcoFalke commented at 7:59 am on September 12, 2022: member

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.

Remove taproot chain param 2222ea8612

in src/rpc/blockchain.cpp:1333 in faf3b412e0 outdated

1329@@ -1320,6 +1330,9 @@ static RPCHelpMan getdeploymentinfo()
1330                 {RPCResult::Type::OBJ_DYN, "deployments", "", {
1331                     {RPCResult::Type::OBJ, "xxxx", "name of the deployment", RPCHelpForDeployment}
1332                 }},
1333+                {RPCResult::Type::OBJ_DYN, "script_flag_exceptions", "All script flag exceptions for this chain in blocks before or after the tip.", {

Sjors commented at 8:15 am on September 12, 2022:

faf3b412e0316f626ceb15658dd9a017cacbf07e:

for this network. Exempted blocks are not necessarily part of the chain, e.g. during IBD or an extreme reorg.

Sjors commented at 8:26 am on September 12, 2022:

I use the word “exempted” here, hopefully correctly: script flag exceptions determine which blocks are exempted from the default script flags.

MarcoFalke commented at 8:36 am on September 12, 2022:

thx, done

ajtowns commented at 10:22 am on September 12, 2022:

exempt goes with exemption, except goes with exception - https://www.differencebetween.com/difference-between-exemption-and-vs-exception/

exempt implies fewer restrictions, an exception simply implies different restrictions. either’s correct here.

Sjors commented at 1:10 pm on September 12, 2022:

I based this on a different explanation: https://english.stackexchange.com/a/419105

But on second read it’s not that clear. Picking one sounds good to me too.

Sjors commented at 8:24 am on September 12, 2022: member

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

MarcoFalke force-pushed on Sep 12, 2022

DrahtBot removed the label Needs rebase on Sep 12, 2022

rpc: Report script_flag_exceptions in getdeploymentinfo fa7b5ca198

MarcoFalke force-pushed on Sep 12, 2022

ajtowns commented at 12:58 pm on September 12, 2022: contributor

Don’t really like the idea of reporting the exceptions without reporting what’s “normal”.

I was thinking more along the lines of: https://github.com/ajtowns/bitcoin/commit/809a545d6e9abb106d5345f3a98ebc7b4031631b (possibly reporting bip16 support/exceptions as well) which gives output like:

 0{
 1  "hash": "00000000000000000007e4449a4d61ba2d2a3f0bbc0bbf355d2fcc8deecdb864",
 2  "height": 753789,
 3  "deployments": {
 4    ...
 5    "bip16": {
 6      "type": "buried",
 7      "buried": {
 8        "exceptions": [
 9          "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22"
10        ]
11      },
12      "active": true,
13      "height": 0
14    },
15    ...
16    "segwit": {
17      "type": "buried",
18      "active": true,
19      "height": 481824
20    },
21    "taproot": {
22      "type": "buried",
23      "buried": {
24        "exceptions": [
25          "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
26          "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad"
27        ]
28      },
29      "active": true,
30      "height": 0
31    }
32  }
33}

MarcoFalke commented at 1:31 pm on September 13, 2022: member

Ok, leaving up for grabs for now

MarcoFalke closed this on Sep 13, 2022

MarcoFalke deleted the branch on Sep 13, 2022

Sjors commented at 4:37 pm on September 23, 2022: member

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

bitcoin locked this on Sep 23, 2023

Remove taproot chain param #24737

Conflicts