Fix certificate verification by plebhash · Pull Request #752 · stratum-mining/stratum

plebhash · 2024-02-14T14:39:35Z

replacement for lorbax#3

plebhash · 2024-02-14T14:40:44Z

todos:

add the possibility to leave the auth pub key for the TP in the pool set to None (void string).
~~make it mandatory on mainnet~~ (moved to make TP authority key mandatory for mainnet sv2-apps#10)

github-actions · 2024-02-14T14:43:45Z

Bencher

Report	Fri, February 16, 2024 at 15:47:49 UTC
Project	Stratum v2 (SRI)
Branch	fix-certificate-verify
Testbed	sv1

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)
client-submit-serialize	✅ (view plot)	6710.900
client-submit-serialize-deserialize	✅ (view plot)	7696.600
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle	✅ (view plot)	8292.600
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle	✅ (view plot)	889.210
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize	✅ (view plot)	691.650
client-sv1-authorize-serialize/client-sv1-authorize-serialize	✅ (view plot)	244.470
client-sv1-get-authorize/client-sv1-get-authorize	✅ (view plot)	155.790
client-sv1-get-submit	✅ (view plot)	6606.300
client-sv1-get-subscribe/client-sv1-get-subscribe	✅ (view plot)	285.320
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	✅ (view plot)	753.420
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize	✅ (view plot)	618.780
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize	✅ (view plot)	212.240

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-02-14T14:43:45Z

Bencher

Report	Fri, February 16, 2024 at 15:47:49 UTC
Project	Stratum v2 (SRI)
Branch	fix-certificate-verify
Testbed	sv2

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)
client_sv2_handle_message_common	✅ (view plot)	43.954
client_sv2_handle_message_mining	✅ (view plot)	64.486
client_sv2_mining_message_submit_standard	✅ (view plot)	14.668
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	272.210
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	573.690
client_sv2_open_channel	✅ (view plot)	172.130
client_sv2_open_channel_serialize	✅ (view plot)	298.170
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	369.820
client_sv2_setup_connection	✅ (view plot)	162.180
client_sv2_setup_connection_serialize	✅ (view plot)	478.690
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	941.660

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-02-14T14:43:47Z

Bencher

Report	Fri, February 16, 2024 at 15:47:53 UTC
Project	Stratum v2 (SRI)
Branch	fix-certificate-verify
Testbed	sv1

Benchmark	Estimated Cycles	Estimated Cycles Results estimated cycles \| (Δ%)	Instructions	Instructions Results instructions \| (Δ%)	L1 Accesses	L1 Accesses Results accesses \| (Δ%)	L2 Accesses	L2 Accesses Results accesses \| (Δ%)	RAM Accesses	RAM Accesses Results accesses \| (Δ%)
get_authorize	✅ (view plot)	8489.000	✅ (view plot)	3772.000	✅ (view plot)	5294.000	✅ (view plot)	9.000	✅ (view plot)	90.000
get_submit	✅ (view plot)	95664.000	✅ (view plot)	59522.000	✅ (view plot)	85489.000	✅ (view plot)	61.000	✅ (view plot)	282.000
get_subscribe	✅ (view plot)	8062.000	✅ (view plot)	2848.000	✅ (view plot)	3977.000	✅ (view plot)	19.000	✅ (view plot)	114.000
serialize_authorize	✅ (view plot)	12314.000	✅ (view plot)	5343.000	✅ (view plot)	7454.000	✅ (view plot)	13.000	✅ (view plot)	137.000
serialize_deserialize_authorize	✅ (view plot)	24482.000	✅ (view plot)	9950.000	✅ (view plot)	14052.000	✅ (view plot)	35.000	✅ (view plot)	293.000
serialize_deserialize_handle_authorize	✅ (view plot)	30070.000	✅ (view plot)	12127.000	✅ (view plot)	17170.000	✅ (view plot)	60.000	✅ (view plot)	360.000
serialize_deserialize_handle_submit	✅ (view plot)	126487.000	✅ (view plot)	73307.000	✅ (view plot)	105087.000	✅ (view plot)	122.000	✅ (view plot)	594.000
serialize_deserialize_handle_subscribe	✅ (view plot)	27396.000	✅ (view plot)	9650.000	✅ (view plot)	13651.000	✅ (view plot)	68.000	✅ (view plot)	383.000
serialize_deserialize_submit	✅ (view plot)	115279.000	✅ (view plot)	68167.000	✅ (view plot)	97834.000	✅ (view plot)	73.000	✅ (view plot)	488.000
serialize_deserialize_subscribe	✅ (view plot)	22896.000	✅ (view plot)	8209.000	✅ (view plot)	11566.000	✅ (view plot)	40.000	✅ (view plot)	318.000
serialize_submit	✅ (view plot)	100045.000	✅ (view plot)	61566.000	✅ (view plot)	88335.000	✅ (view plot)	60.000	✅ (view plot)	326.000
serialize_subscribe	✅ (view plot)	11455.000	✅ (view plot)	4195.000	✅ (view plot)	5835.000	✅ (view plot)	18.000	✅ (view plot)	158.000

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-02-14T14:43:53Z

Bencher

Report	Fri, February 16, 2024 at 15:47:50 UTC
Project	Stratum v2 (SRI)
Branch	fix-certificate-verify
Testbed	sv2

Benchmark	Estimated Cycles	Estimated Cycles Results estimated cycles \| (Δ%)	Instructions	Instructions Results instructions \| (Δ%)	L1 Accesses	L1 Accesses Results accesses \| (Δ%)	L2 Accesses	L2 Accesses Results accesses \| (Δ%)	RAM Accesses	RAM Accesses Results accesses \| (Δ%)
client_sv2_handle_message_common	✅ (view plot)	2055.000	✅ (view plot)	469.000	✅ (view plot)	730.000	✅ (view plot)	6.000	✅ (view plot)	37.000
client_sv2_handle_message_mining	✅ (view plot)	8215.000	✅ (view plot)	2075.000	✅ (view plot)	3055.000	✅ (view plot)	45.000	✅ (view plot)	141.000
client_sv2_mining_message_submit_standard	✅ (view plot)	6538.000	✅ (view plot)	1750.000	✅ (view plot)	2543.000	✅ (view plot)	22.000	✅ (view plot)	111.000
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	14733.000	✅ (view plot)	4694.000	✅ (view plot)	6748.000	✅ (view plot)	57.000	✅ (view plot)	220.000
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	27416.000	✅ (view plot)	10537.000	✅ (view plot)	15326.000	✅ (view plot)	94.000	✅ (view plot)	332.000
client_sv2_open_channel	✅ (view plot)	4557.000	✅ (view plot)	1461.000	✅ (view plot)	2152.000	✅ (view plot)	12.000	✅ (view plot)	67.000
client_sv2_open_channel_serialize	✅ (view plot)	14010.000	✅ (view plot)	5064.000	✅ (view plot)	7320.000	✅ (view plot)	43.000	✅ (view plot)	185.000
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	22452.000	✅ (view plot)	7979.000	✅ (view plot)	11612.000	✅ (view plot)	75.000	✅ (view plot)	299.000
client_sv2_setup_connection	✅ (view plot)	4749.000	✅ (view plot)	1502.000	✅ (view plot)	2274.000	✅ (view plot)	12.000	✅ (view plot)	69.000
client_sv2_setup_connection_serialize	✅ (view plot)	16108.000	✅ (view plot)	5963.000	✅ (view plot)	8658.000	✅ (view plot)	48.000	✅ (view plot)	206.000
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	35446.000	✅ (view plot)	14806.000	✅ (view plot)	21746.000	✅ (view plot)	101.000	✅ (view plot)	377.000

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Sjors · 2024-02-14T14:48:56Z

add the possibility to leave the auth pub key for the TP in the pool set to None (void string)

No need to do that in this PR.

This PR depends on #737. It can be rebased once that's merged.

Sjors

ACK

Tested against sjors/sv2 with commit e477666694954d6a011dcd5e3188851651a4e011 reverted.

Just ping me when this is merged so I can update sjors/sv2 to match.

CI is expected to fail since it uses sjors/sv2 without the revert commit.

I did not check the example config files.

Sjors · 2024-02-15T14:16:50Z

Just tested that 3b5cd45 still works against my sv2 branch. Hopefully CI can be tamed so we can merge this...

Fi3 · 2024-02-15T17:01:59Z

add the possibility to leave the auth pub key for the TP in the pool set to None (void string)

No need to do that in this PR.

This PR depends on #737. It can be rebased once that's merged.

We need it in that PR otherwise after merging it will have a broken main, and it will become very hard to run the roles and test things out

Fi3 · 2024-02-15T17:08:52Z

Also note that some MG test need an hosted TP if the hosted TP go down and we need to restart it we would also need to change the config files of the tested roles to put the new key it could very easly becom a nightmare

Sjors

ACK 3af5a04

Don't worry about making mainnet mandatory.

I tested that it can connect to the Template Provider:

with tp_authority_public_key set
without tp_authority_public_key (new since my last review)

I also checked that if I used the wrong public key, it refuses.

CI no longer checks the TP certificate. That's understandable given that it needs to be manually updated, but it could cause a future regression to missed. Though as long as someone uses it, we'll learn quickly enough.

Sjors · 2024-02-16T08:11:45Z

protocols/v2/noise-sv2/src/initiator.rs

+                }
+            }
+            None => {
+                let (temp_k1, temp_k2) = Self::hkdf_2(self.get_ck(), &[]);


This should be de-duplicated (in a followup).

I wrote a comment that is equivalent to the yours, sorry

lorbax · 2024-02-16T09:36:21Z

protocols/v2/noise-sv2/src/initiator.rs

-            Err(Error::InvalidCertificate(plaintext))
+        match &self.responder_authority_pk {
+            Some(responder_authority_pk) => {
+                if signature_message.verify(&rs_pk_xonly, responder_authority_pk) {


I think that it would be better to move the match inside the .verify() method, uin order to have a cleaner step2 code. Something like

pub fn verify(self, pk_: &Option<XOnlyPublicKey>) -> bool { match pk_ { Some(pk) => { let now = SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap() .as_secs() as u32; if self.valid_from <= now && self.not_valid_after >= now { let secp = Secp256k1::verification_only(); let (m, s) = self.split(); let m = Message::from_hashed_data::<sha256::Hash>(&m[0..10]); let s = match Signature::from_slice(&s) { Ok(s) => s, _ => return false, }; secp.verify_schnorr(&s, &m, pk).is_ok() } else { false } }, None => true } }

done eff00b3

lorbax · 2024-02-16T12:04:54Z

roles/jd-client/src/lib/template_receiver/mod.rs


-        let pub_key: Secp256k1PublicKey = authority_public_key;
-        let initiator = Initiator::from_raw_k(pub_key.into_bytes()).unwrap();
+        let initiator = match authority_public_key {


Here you can turn authority_public_key: Option<Secp256k1PublicKey> in a variable authority_public_key_x_only: Option<XOnlyPublicKey> and initialize the initiator with Initiator::new(autority_public_key_x_only). You save some lines of code.
I understand that you didn't write the whole code, but perhaps the methods Initiator::from_raw_k() and Initiator::without_pk() can be eliminated.

while I agree this would be a desirable improvement to the code, this would imply refactoring multiple parts of the codebase which don't really have much to do with the context of this PR

so in order to keep PR atomicity, I created this issue so we can implement this improvement in the future:
#760

lorbax · 2024-02-16T12:12:20Z

roles/pool/config-examples/pool-config-local-tp-example.toml

 # Template Provider config
 # Local TP (this is pointing to localhost so you must run a TP locally for this configuration to work)
 tp_address = "127.0.0.1:8442"
+# You'll need to get tp_authority_public_key from the logs of your TP, for example:


The comment "You'll need to get tp_authority_public_key from the logs of your TP" is against what I understood to have agreed: if you run a local TP yuu clearly trust it and there is no need of certificate authority (and no need to retrive that from TP logs)

In this example the TP is local, but it doesn't have to be. Maybe change "You'll need to" to "You may" and mention that it's not necessary for a TP on localhost.

I know the example file is "local-tp", but afaik we don't have any example of a remote TP, so it seems better to leave a comment in the README.

made some changes in 1a48fa5

lmk what you guys think

Not sure if it make sense as a comment. You know the auth pub key if you see it in the log and TP is local it means that you set the TP with the auth pub key so you have to know it. I think that this comment is can confuse the reader.

bitcoind go down she must resend the key to Bob.

Bitcoin Core stores the key. Only if Alice deletes the data directory, and not have a backup, would she have to generate a new key.

The key is stored in ~/.bitcoin/sv2_authority_key so it's trivial to make a backup.

ok this make sense to me

Yes but there is no need to retrieve is from the log every time. It is better to let decide the user to decide whenever to use a new authority key. I would say also that could be a little annoying to do it manually. I like the -sv2cert solution btw!

What do you mean by "every time"? In the above scenario Alice needs to do that once. The key doesn't change (unless the user deletes the key or the whole data dir).

Some messages didn't appear to me at the moment of writing my last message. The idea of generating the key and storing it seems good to me and I ack it. In order to reuse an older key (from a previous backup) we should still be able to pass it as an argument to the bitcoind, like the -sv2cert idea.

lorbax · 2024-02-16T12:17:10Z

roles/pool/src/lib/template_receiver/mod.rs


-        let pub_key: Secp256k1PublicKey = authority_public_key;
-        let initiator = Initiator::from_raw_k(pub_key.into_bytes())?;
+        let initiator = match expected_tp_authority_public_key {


same thing of the previous comment

Sjors

Tested that eff00b3 still works.

protocols/v2/noise-sv2/src/signature_message.rs

Sjors

Untested ACK b4a4387

Let's leave "cosmetic" changes to a followup so we can merge this breaking change.

Sjors · 2024-02-16T19:02:28Z

🎉

I updated my sv2 branch to reflect this change (i.e. to drop the workaround).

plebhash mentioned this pull request Feb 14, 2024

Fix certificate verification lorbax/stratum#3

Closed

plebhash force-pushed the fix-certificate-verify branch from d376d3f to 3b5cd45 Compare February 14, 2024 18:15

Sjors approved these changes Feb 14, 2024

View reviewed changes

plebhash marked this pull request as ready for review February 15, 2024 11:46

plebhash force-pushed the fix-certificate-verify branch from fce947a to 3b5cd45 Compare February 15, 2024 13:28

plebhash marked this pull request as draft February 15, 2024 19:47

plebhash force-pushed the fix-certificate-verify branch 3 times, most recently from 694e83d to 28df4e1 Compare February 15, 2024 21:19

Sjors approved these changes Feb 16, 2024

View reviewed changes

lorbax reviewed Feb 16, 2024

View reviewed changes

plebhash force-pushed the fix-certificate-verify branch 4 times, most recently from 2591a9b to eff00b3 Compare February 16, 2024 11:41

plebhash mentioned this pull request Oct 28, 2025

make TP authority key mandatory for mainnet stratum-mining/sv2-apps#10

Open

plebhash marked this pull request as ready for review February 16, 2024 11:46

lorbax reviewed Feb 16, 2024

View reviewed changes

Sjors approved these changes Feb 16, 2024

View reviewed changes

plebhash mentioned this pull request Feb 16, 2024

simplify Initiator constructors #760

Closed

pavlenex mentioned this pull request Feb 16, 2024

Make authority public key optional? #746

Closed

Fi3 reviewed Feb 16, 2024

View reviewed changes

protocols/v2/noise-sv2/src/signature_message.rs Outdated Show resolved Hide resolved

plebhash added 16 commits February 16, 2024 12:42

fix certificate validation

dddafd9

add clarification comment for certificate verification

ec0785c

rename tp_authority_public_key variables

b5492f4

fix comment

095dc87

verify certificate against authority_pk

251baa4

rename responder_authority_pk

7f53874

fix jd-client for new TP authority certificate validation

559b789

add comments for TP authority key

570d827

add tp_authority_public_key to test configs

bb1a1c2

clippy

310d4e4

change tp_authority_public_key for tests

ef75b40

allow null tp_authority_public_key

b654473

remove tp_authority_public_key from test configs

f61cc95

move match inside verify

e3712be

improve comments around tp_authority_public_key

a1cf7b3

replace match with if let

b4a4387

plebhash force-pushed the fix-certificate-verify branch from bb6520b to b4a4387 Compare February 16, 2024 15:42

Sjors approved these changes Feb 16, 2024

View reviewed changes

pavlenex requested a review from Fi3 February 16, 2024 17:51

Fi3 approved these changes Feb 16, 2024

View reviewed changes

Sjors mentioned this pull request Feb 16, 2024

new rpc-client for JD-Server mempool #761

Merged

pavlenex merged commit c4d9f59 into stratum-mining:main Feb 16, 2024

Sjors mentioned this pull request Feb 16, 2024

Stratum v2 Template Provider (take 3) bitcoin/bitcoin#29432

Closed

24 tasks

plebhash deleted the fix-certificate-verify branch February 16, 2024 19:14

Sjors mentioned this pull request Mar 8, 2024

Stratum v2 handshake fails cert validation Sjors/bitcoin#38

Closed

1 task

Comments

Conversation

plebhash commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plebhash commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher

Uh oh!

github-actions bot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher

Uh oh!

Sjors commented Feb 14, 2024

Uh oh!

Sjors left a comment • 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 commented Feb 15, 2024

Uh oh!

Fi3 commented Feb 15, 2024

Uh oh!

Fi3 commented Feb 15, 2024

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorbax Feb 16, 2024 • 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!

plebhash Feb 16, 2024 • 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!

lorbax Feb 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

lorbax Feb 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

plebhash commented Feb 14, 2024 •

edited

Loading

plebhash commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

Sjors left a comment •

edited

Loading

lorbax Feb 16, 2024 •

edited

Loading

plebhash Feb 16, 2024 •

edited

Loading

lorbax Feb 16, 2024 •

edited

Loading

lorbax Feb 16, 2024 •

edited

Loading

Sjors Feb 16, 2024 •

edited

Loading