Elligator swift encoding #737

lorbax · 2024-01-29T11:23:22Z

ElligatorSwift encoding for pubkeys in handshake operations.
This branch must be tested with thev new TP by @Sjors.

Already tested in regtest. Needs to be tested in testnet.

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

protocols/v2/codec-sv2/src/lib.rs

protocols/v2/const-sv2/src/lib.rs

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

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

Sjors · 2024-01-29T12:46:00Z

One more thing you'll need to be able to communicate with the Bitcoin Core TP... I changed the protocol name to Noise_NX_EllSwiftXonly_ChaChaPoly_SHA256, so this changes the PROTOCOL_NAME_HASH. You can see the corresponding byte values in my PR, but it might be better if you generate them independently - as a sanity check.

Sjors · 2024-02-06T12:29:31Z

Maybe squash the first 5 commits? It's easier to read when every commit does something specific, like 67c1b7a and 12356bd.

You could also make a separate commit that bumps libsecp to 0.28.2. That way you remove some "noise" from the main changes.

protocols/v2/const-sv2/src/lib.rs

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

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

Sjors · 2024-02-06T12:42:18Z

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

-        encrypted_static_pub_k[..32].copy_from_slice(&static_pub_k[..32]);
+        // 5. appends `EncryptAndHash(s.public_key)` (64 bytes encrypted elligatorswift  public key, 16 bytes MAC)
+        let mut encrypted_static_pub_k = vec![0; ELLSWIFT_ENCODING_SIZE];
+        let elligatorswift_ours_static = ElligatorSwift::from_pubkey(self.s.public_key());


It is possible to get the EllSwift encoded key directly from the private key?

That might be a bit faster. Though performance is not a big deal here.

Yes we can derive the Ellswift pubkey from the private key. Moreover, in the SRI internal representation of Initiator and Responder handshake roles it is possible to store only the private key and derive whatever we want from that. This goes even further.
I am not totally against it, but I would prefer to convert the public key instead for two reasons

because it resembles more the idea that then Ellswift is just an encoding

I would like to think about it, test it and generally be more conservative an proceed by steps

What do you think about?

I don't have a strong preference.

Sjors · 2024-02-06T12:44:45Z

Tested that this works against https://github.com/Sjors/bitcoin/tree/2024/02/sv2-poll-ellswift

Left some cosmetic comments above.

It should now be possible to simplify generate_key and drop its x_only_public_key().1 == crate::PARITY check. This would be a good thing to test.

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

Sjors · 2024-02-09T09:08:10Z

@lorbax based on discussion in stratum-mining/sv2-spec#66 (comment) I changed the protocol name to Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256.

However, in order not to break your pull request here, I temporarily added a revert commit which keeps the old protocol name Noise_NX_EllSwiftXonly_ChaChaPoly_SHA256: Sjors/bitcoin@f65b4ef (included in the 2024/02/sv2-poll-ellswift branch)

Let me know when you've had a chance to change the protocol name here, and I'll drop that commit.

github-actions · 2024-02-09T09:22:15Z

Bencher

Report	Wed, February 14, 2024 at 10:19:09 UTC
Project	Stratum v2 (SRI)
Branch	ElligatorSwift
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)	8515.000	✅ (view plot)	3772.000	✅ (view plot)	5295.000	✅ (view plot)	7.000	✅ (view plot)	91.000
get_submit	✅ (view plot)	95662.000	✅ (view plot)	59522.000	✅ (view plot)	85497.000	✅ (view plot)	52.000	✅ (view plot)	283.000
get_subscribe	✅ (view plot)	8092.000	✅ (view plot)	2848.000	✅ (view plot)	3977.000	✅ (view plot)	18.000	✅ (view plot)	115.000
serialize_authorize	✅ (view plot)	12352.000	✅ (view plot)	5343.000	✅ (view plot)	7452.000	✅ (view plot)	14.000	✅ (view plot)	138.000
serialize_deserialize_authorize	✅ (view plot)	24532.000	✅ (view plot)	9950.000	✅ (view plot)	14047.000	✅ (view plot)	39.000	✅ (view plot)	294.000
serialize_deserialize_handle_authorize	✅ (view plot)	30130.000	✅ (view plot)	12127.000	✅ (view plot)	17170.000	✅ (view plot)	58.000	✅ (view plot)	362.000
serialize_deserialize_handle_submit	✅ (view plot)	126509.000	✅ (view plot)	73307.000	✅ (view plot)	105089.000	✅ (view plot)	119.000	✅ (view plot)	595.000
serialize_deserialize_handle_subscribe	✅ (view plot)	27430.000	✅ (view plot)	9650.000	✅ (view plot)	13650.000	✅ (view plot)	68.000	✅ (view plot)	384.000
serialize_deserialize_submit	✅ (view plot)	115289.000	✅ (view plot)	68167.000	✅ (view plot)	97839.000	✅ (view plot)	67.000	✅ (view plot)	489.000
serialize_deserialize_subscribe	✅ (view plot)	22930.000	✅ (view plot)	8209.000	✅ (view plot)	11565.000	✅ (view plot)	40.000	✅ (view plot)	319.000
serialize_submit	✅ (view plot)	100047.000	✅ (view plot)	61566.000	✅ (view plot)	88342.000	✅ (view plot)	52.000	✅ (view plot)	327.000
serialize_subscribe	✅ (view plot)	11485.000	✅ (view plot)	4195.000	✅ (view plot)	5835.000	✅ (view plot)	17.000	✅ (view plot)	159.000

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

github-actions · 2024-02-09T09:22:15Z

Bencher

Report	Wed, February 14, 2024 at 10:19:05 UTC
Project	Stratum v2 (SRI)
Branch	ElligatorSwift
Testbed	sv1

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)
client-submit-serialize	✅ (view plot)	6656.900
client-submit-serialize-deserialize	✅ (view plot)	7617.300
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle	✅ (view plot)	8228.700
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle	✅ (view plot)	909.170
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize	✅ (view plot)	691.320
client-sv1-authorize-serialize/client-sv1-authorize-serialize	✅ (view plot)	245.340
client-sv1-get-authorize/client-sv1-get-authorize	✅ (view plot)	156.350
client-sv1-get-submit	✅ (view plot)	6420.800
client-sv1-get-subscribe/client-sv1-get-subscribe	✅ (view plot)	280.570
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	✅ (view plot)	767.440
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize	✅ (view plot)	607.120
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize	✅ (view plot)	208.610

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

github-actions · 2024-02-09T09:22:16Z

Bencher

Report	Wed, February 14, 2024 at 10:19:05 UTC
Project	Stratum v2 (SRI)
Branch	ElligatorSwift
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)	1957.000	✅ (view plot)	469.000	✅ (view plot)	732.000	✅ (view plot)	7.000	✅ (view plot)	34.000
client_sv2_handle_message_mining	✅ (view plot)	8097.000	✅ (view plot)	2081.000	✅ (view plot)	3062.000	✅ (view plot)	48.000	✅ (view plot)	137.000
client_sv2_mining_message_submit_standard	✅ (view plot)	6310.000	✅ (view plot)	1756.000	✅ (view plot)	2555.000	✅ (view plot)	23.000	✅ (view plot)	104.000
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	14791.000	✅ (view plot)	4700.000	✅ (view plot)	6756.000	✅ (view plot)	53.000	✅ (view plot)	222.000
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	27506.000	✅ (view plot)	10543.000	✅ (view plot)	15326.000	✅ (view plot)	98.000	✅ (view plot)	334.000
client_sv2_open_channel	✅ (view plot)	4353.000	✅ (view plot)	1461.000	✅ (view plot)	2158.000	✅ (view plot)	12.000	✅ (view plot)	61.000
client_sv2_open_channel_serialize	✅ (view plot)	14154.000	✅ (view plot)	5064.000	✅ (view plot)	7314.000	✅ (view plot)	45.000	✅ (view plot)	189.000
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	22488.000	✅ (view plot)	7979.000	✅ (view plot)	11603.000	✅ (view plot)	84.000	✅ (view plot)	299.000
client_sv2_setup_connection	✅ (view plot)	4693.000	✅ (view plot)	1502.000	✅ (view plot)	2273.000	✅ (view plot)	15.000	✅ (view plot)	67.000
client_sv2_setup_connection_serialize	✅ (view plot)	16172.000	✅ (view plot)	5963.000	✅ (view plot)	8657.000	✅ (view plot)	47.000	✅ (view plot)	208.000
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	35506.000	✅ (view plot)	14806.000	✅ (view plot)	21746.000	✅ (view plot)	99.000	✅ (view plot)	379.000

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

github-actions · 2024-02-09T09:22:17Z

Bencher

Report	Wed, February 14, 2024 at 10:19:03 UTC
Project	Stratum v2 (SRI)
Branch	ElligatorSwift
Testbed	sv2

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)
client_sv2_handle_message_common	✅ (view plot)	44.224
client_sv2_handle_message_mining	✅ (view plot)	68.023
client_sv2_mining_message_submit_standard	✅ (view plot)	14.666
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	262.650
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	576.200
client_sv2_open_channel	✅ (view plot)	166.860
client_sv2_open_channel_serialize	✅ (view plot)	284.350
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	377.700
client_sv2_setup_connection	✅ (view plot)	158.760
client_sv2_setup_connection_serialize	✅ (view plot)	483.600
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	945.520

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

lorbax · 2024-02-10T18:14:41Z

fixed history and force-pushed
@Sjors how does it look like the commits' history now?

Sjors · 2024-02-12T09:44:53Z

I don't think "most tests pass" and "compiles but doesn't work" are useful commit names.

I think two commits should be enough:

Update secp256k1 to v0.28.2
Use EllSwift encoding and ECDH

lorbax · 2024-02-12T10:07:47Z

@Sjors I will do it today

Sjors · 2024-02-12T10:17:34Z

There's also one failing test: https://github.com/stratum-mining/stratum/actions/runs/7856601115/job/21439624655?pr=737#step:7:746

GitGab19 · 2024-02-12T10:25:58Z

There's also one failing test: https://github.com/stratum-mining/stratum/actions/runs/7856601115/job/21439624655?pr=737#step:7:746

It's because on our VPS is currently running the 2023/11/sv2-poll branch of your TP. We changed it again because otherwise every other past PRs would have not passed CI tests.
Let me know when it's ready to be tested, I will update our hosted TP with latest 2024/02/sv2-poll-ellswift so that MG tests can be correctly executed

Sjors · 2024-02-12T15:36:45Z

Tested 109f233 (with #747 cherry picked) against the template provider 2024/02/sv2-poll-ellswift (commit 36d2887071e64e6fb052e656afbd423506ae6fdd). The connection is established and messages are sent back and forth, so that's great.

Code looks correct too.

Minor nit: the ElligatorSwift encoding commit still contains some unrelated changes in clippy-on-all-workspaces.sh and some Cargo.lock files.

@Fi3 / @jakubtrnka can you give it a code review as well?

I will update our hosted TP with latest 2024/02/sv2-poll-ellswift so that MG tests can be correctly executed

Once this is merged I plan to update 2023/11/sv2-poll to match 2024/02/sv2-poll-ellswift, so that last branch won't be needed anymore.

lorbax · 2024-02-12T17:30:42Z

@Sjors restored cargo lock in utils
clippy-on-all-workspaces.sh should have just a minor modification
Now I think that it can be reviewed by someone else

Fi3 · 2024-02-13T08:22:19Z

protocols/v2/const-sv2/Cargo.toml

@@ -10,6 +10,7 @@ repository = "https://github.com/stratum-mining/stratum"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
+secp256k1 = { version = "0.28.2", default-features = false, features =["hashes", "alloc","rand","rand-std"] }


seems that you can remove this dependency

What do you think about this?
pub const ELLSWIFT_ENCODING_SIZE: usize = secp256k1::constants::ELLSWIFT_ENCODING_SIZE;
I thought it would have been a good idea to keep this constant depending from the one in secp256kq (it is what I meant when I added it in the toml, but later I forgot).
Otherwise, just
pub const ELLSWIFT_ENCODING_SIZE: usize = 64;
without calling secp256k1 library.

Fi3 · 2024-02-13T08:35:51Z

roles/jd-client/Cargo.toml

@@ -29,4 +29,4 @@ tracing-subscriber = { version = "0.3" }
 error_handling = { version = "0.1", path = "../../utils/error-handling" }
 nohash-hasher = "0.2.0"
 key-utils = { version = "^1.0.0", path = "../../utils/key-utils" }
-secp256k1 = { version = "0.27.0", default-features = false, features =["bitcoin_hashes","alloc","rand","rand-std"] }
+secp256k1 = { version = "0.28.2", default-features = false, features =["alloc","rand","rand-std"] }


Maybe we should put it in the stratum-common crate and import secp256k1 everywhere from there?

I was thinking also that using secp256k1 in the roles is not the best thing. This logic should stay in roles-logic-sv2 or key-utils. But I guess that this will be better suited for a new PR.

I just checked that sepc256k1 in roles is used in

jd-server for signing the tokens

pool to verify tokens

So I think that it is unavoidable importing there. Btw it is not used in any other role (and therefore I can remove it from the jd-client dependencies).
I also agree that we should put secp256k1 in stratum common import from there

Sjors

Tested 1c42de9. Only difference with my last review is in a Cargo.lock file.

Use ElligatorSwift encoding for representations of public keys and the related ECDH procedure For serializing and deserilizing the messages sxchanges during the handshake is made use of constants for lenghts of message chunks change protocol name for encryption New protocol name "Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256" downstream_sv1::diff_management::test::test_converge_to_spm_from_low downstream_sv1::diff_management::test::test_diff_management fail non-deterministically On top of https://github.com/GitGab19/stratum/tree/fix-empty-mempool

in jd-client role was imported secp256k1 library, but wasn't used in that crate.

Sjors

Tested the rebase, ACK b6008d4