Feat/deps update #268

dzdidi · 2025-02-03T14:39:11Z

Update dependencies for lightning and bitcoin as a prerequisite for #133

The suggested change is mostly brute-force update. Suggestions for improvements of overall quality are very appreciated.

I'll clean history and apply other changes upon completion of review unless requested otherwise

Signed-off-by: dzdidi <[email protected]>

mariocynicys · 2025-02-07T19:56:44Z

teos/src/bitcoin_cli.rs

+        let binding =
+            general_purpose::URL_SAFE.encode(format!("{}:{}", self.rpc_user, self.rpc_password));
+        let rpc_credentials = binding.as_str();
+        Ok(RpcClient::new(&rpc_credentials, http_endpoint))


no need for the .as_str() line.
&String will be tread as &str by the compiler.

mariocynicys

Thanks!

Could you please fix the lint errors and cln-plugin test errors?

Looks like these changes bumped the MSRV. Not that it's really important to support older versions, but we can now state this version in:

rust-teos/DEPENDENCIES.md

Line 9 in 4e47db2

FIXME: Define MSRV

and also create a rust-toolchain.toml file with the MSRV.
This doesn't have to be done in this PR.

I did a light review for now and skipped the changes in the watchtower plugin, but curious why did you update the dependencies there?

The new imports I think need a bit of a rework.

Some imports losing context when used (e.g. let binding = general_purpose::URL_SAFE.encode(format!("{}:{}", self.rpc_user, self.rpc_password));, here general_purpose doesn't really tell that this is doing base64 encoding, a clearer import for instance would be general_purpose::URL_SAFE as base64_encoder)

And some other imports could be added to make type usages less lengthy (e.g. bitcoin::block::Version::from_consensus(0) has a long path, why not let it block::Version::from_consensus(0) and import bitcoin::block at the top).

mariocynicys · 2025-02-07T20:07:14Z

teos/src/bitcoin_cli.rs

@@ -146,7 +151,7 @@ impl<'a> BitcoindClient<'a> {
    pub async fn send_raw_transaction(&self, raw_tx: &Transaction) -> Result<Txid, std::io::Error> {
        let rpc = self.bitcoind_rpc_client.lock().await;

-        let raw_tx_json = serde_json::json!(raw_tx.encode().to_hex());
+        let raw_tx_json = serde_json::json!(raw_tx.encode());


why is this value no longer hexed? does sendrawtransaction accept a vector of bytes now?

mariocynicys · 2025-02-07T20:23:10Z

teos-common/src/test_utils.rs

+    // FIXME: might be cleaner?
+    // Add 1 to make sure it is not 0
+    let size = (get_random_int::<usize>() % 81) + 1;
+    let random_bytes = cryptography::get_random_bytes(size);
+    let mut fixed_bytes = [0u8; 81];
+    fixed_bytes[..random_bytes.len()].copy_from_slice(&random_bytes);
+    let script_string = format!("6a{}", hex::encode(random_bytes));
+    let script_pubkey = ScriptBuf::from(hex::decode(script_string).unwrap());


we can avoid doing this by hand. ScriptBuf::new_op_return should do the job.

also the size here might shoot to 81 with the extra +1 added here, which would make this a bad tx.
no need for the +1, opreturns wont complain with zero bytes

teos/src/tx_index.rs

mariocynicys · 2025-02-07T20:33:17Z

teos/src/tx_index.rs

+            match block.deref() {
+                lightning_block_sync::BlockData::HeaderOnly(_) => {}
+                lightning_block_sync::BlockData::FullBlock(block) => {
+                    let map = block
+                        .txdata
+                        .iter()
+                        .map(|tx| {
+                            (
+                                K::from_txid(tx.compute_txid()),
+                                match V::get_type() {
+                                    Type::Transaction => {
+                                        V::from_data(Data::Transaction(tx.clone()))
+                                    }
+                                    Type::BlockHash => {
+                                        V::from_data(Data::BlockHash(block.header.block_hash()))
+                                    }
+                                },
+                            )
+                        })
+                        .collect();
+
+                    tx_index.update(block.header, &map);
+                }
+            }


ummmm, we shouldn't be getting any header only responses as they can't help us populate the index.
if one of these last_n_block is a header only, the check for previous hash will panic.

mariocynicys · 2025-02-07T20:40:02Z

teos/src/tx_index.rs

+        match first_block.deref() {
+            lightning_block_sync::BlockData::FullBlock(b) => {
+                assert_eq!(
+                    cache.get_height(&b.header.block_hash()).unwrap(),
+                    height - cache_size + 1
+                );
+            }
+            _ => panic!("Expected FullBlock"),
+        }
+


aha i see u already spotted the problem with header only blocks. lets panic inside the construct of TxIndex instead.

mariocynicys · 2025-02-07T20:46:49Z

teos/src/responder.rs

+            dispute_txid: t.dispute_tx.compute_txid().to_raw_hash().encode(),
+            penalty_txid: t.penalty_tx.compute_txid().to_raw_hash().encode(),


i think the .to_raw_hash() part could be omitted.

mariocynicys · 2025-02-07T20:49:14Z

teos/src/extended_appointment.rs

+                .try_into()
+                .unwrap(),


i think .try_into.unwrap() could be omitted

mariocynicys · 2025-02-07T20:51:45Z

teos/src/carrier.rs

+        if let Some(receipt) = self.issued_receipts.get(&tx.compute_txid()) {
+            log::info!("Transaction already sent: {}", tx.compute_txid());
            return *receipt;
        }

-        log::info!("Pushing transaction to the network: {}", tx.txid());
+        log::info!("Pushing transaction to the network: {}", tx.compute_txid());
        let receipt = match self.bitcoin_cli.send_raw_transaction(tx) {
            Ok(_) => {
                // Here the transaction could, potentially, have been in mempool before the current height.
                // This shouldn't really matter though.
-                log::info!("Transaction successfully delivered: {}", tx.txid());
+                log::info!("Transaction successfully delivered: {}", tx.compute_txid());


nice to see how we abused using .txid() now that it's apparent it's computed from scratch every time.
this should be tackled in another PR though.

Signed-off-by: dzdidi <[email protected]>

dzdidi · 2025-02-11T13:20:12Z

@mariocynicys thank you for review and feedback!

Unfortunately I'm stuck with CI for CLN plugin is failing. Updating version of cln fixed issues on installation but now I have issues on building default plugins.

Trying to do CI steps on my machine locally makes all my tests fail with the same error on both master and this branch:

----------------------------------------- Captured stdout setup ------------------------------------------
: 2025-02-11T12:44:21.404Z INFO  [teosd] Default data directory: "/tmp/ltests-xp8pia9w/test_get_appointment_1/teos"
: 2025-02-11T12:44:21.404Z INFO  [teosd] Using data directory: "/tmp/ltests-xp8pia9w/test_get_appointment_1/teos/regtest"
: 2025-02-11T12:44:21.404Z INFO  [teosd] Config file: "/tmp/ltests-xp8pia9w/test_get_appointment_1/teos/teos.toml"
: 2025-02-11T12:44:21.404Z INFO  [teos::config] Custom config arg: btc_network: "regtest"
: 2025-02-11T12:44:21.404Z INFO  [teos::config] Custom config arg: btc_rpc_password: ****
: 2025-02-11T12:44:21.404Z INFO  [teos::config] Custom config arg: btc_rpc_port: 40467
: 2025-02-11T12:44:21.404Z INFO  [teos::config] Custom config arg: btc_rpc_user: ****
: 2025-02-11T12:44:21.404Z INFO  [teos::config] Custom config arg: polling_delta: 0
: 2025-02-11T12:44:21.446Z INFO  [teosd] Tower keys not found. Creating a fresh set
: 2025-02-11T12:44:21.483Z INFO  [teosd] tower_id: 0231f3d1f6d90eaf1f9c27649a29bdafc732e030787ff8d261a37b7f440a54c9a5
: 2025-02-11T12:44:21.485Z INFO  [teosd] Current chain tip: 231a685f7e7ec2a975c3ad4112d12cb4d2122a0ef3122a4eca9b6ae23e52945a (height: 101)
: 2025-02-11T12:44:21.614Z INFO  [teosd] Fresh bootstrap
: 2025-02-11T12:44:21.615Z INFO  [teosd] Bootstrap completed. Turning on interfaces
: 2025-02-11T12:44:21.617Z INFO  [teosd] Tower ready
---------------------------------------- Captured stdout teardown ----------------------------------------
: 2025-02-11T12:44:21.801Z INFO  [teosd] Shutting down tower
Leaving base_dir /tmp/ltests-xp8pia9w intact, it still has test sub-directories with failure details: ['test_misbehaving_watchtower_1', 'test_unreachable_watchtower_1', 'test_watchtower_1', 'test_auto_retry_watchtower_1', 'test_get_appointment_1', 'test_manually_retry_watchtower_1']
======================================== short test summary info =========================================
FAILED test.py::test_watchtower - AttributeError: 'NoneType' object has no attribute 'terminate'
FAILED test.py::test_unreachable_watchtower - AttributeError: 'NoneType' object has no attribute 'terminate'
FAILED test.py::test_auto_retry_watchtower - AttributeError: 'NoneType' object has no attribute 'terminate'
FAILED test.py::test_manually_retry_watchtower - AttributeError: 'NoneType' object has no attribute 'terminate'
FAILED test.py::test_misbehaving_watchtower - AttributeError: 'NoneType' object has no attribute 'terminate'
FAILED test.py::test_get_appointment - AttributeError: 'NoneType' object has no attribute 'terminate'
=========================================== 6 failed in 35.10s ===========================================

Logs in /tmp/<base_dir>/<test_name>/teos/log for both master and feature branch look similar enough with corresponding errlog files being empty. I am not exactly sure what to look for at this point so any help will be appreciated

mariocynicys · 2025-02-12T14:38:44Z

@dzdidi thanks. will try to find the issue with CLN tests on the weekend.

dzdidi · 2025-02-19T06:50:09Z

@sangbida since you are interested #133 (comment), do you think #268 (comment) is something you can help with ?

mariocynicys · 2025-02-20T10:37:02Z

@dzdidi thanks. will try to find the issue with CLN tests on the weekend.

sorry for not following along, got plenty of exams these days. will take a look at this on march 1st.

dzdidi · 2025-02-20T11:21:27Z

@mariocynicys , no worries, it is not a blocker. I'm working on client on top of this branch.
Best of luck with exams!

sangbida · 2025-02-20T22:07:55Z

pdating version of cln fixed issues on installation but now I have issues on building default plugins.

Trying to do CI steps on my machine locally makes all my tests fail with the same error on both master and this branch:

Hey yes! I can have a look into this @dzdidi!

sangbida · 2025-02-22T10:27:16Z

pdating version of cln fixed issues on installation but now I have issues on building default plugins.
Trying to do CI steps on my machine locally makes all my tests fail with the same error on both master and this branch:

Hey yes! I can have a look into this @dzdidi!

Just thought I'd do a quick update, I'm still setting up my environment with bitcoind, lnd and eye of satoshi so it may take some time but hoping to get that done asap so I can start looking into this properly.

mariocynicys · 2025-02-22T10:35:48Z

@sangbida

I'm still setting up my environment with bitcoind, lnd and eye of satoshi

you can have a look at the readmes found in the root of the repo and watchtower-plugin. those will help u set up teos, cln, and the cln plugin.

you don't need lnd for this. and it's not supported by teos as of now anyway.

sangbida · 2025-02-22T12:37:05Z

have a look at the readmes found in the root of the repo and watchtower-plugin. those will help u set up teos, cln, and the cln plugin.

That's great to know! I'll skip lnd and get onto teos.

sangbida · 2025-02-25T05:14:49Z

Hey, I was able to get all set up. I think the error here is that the plugin can't find cargo. I've tried to add it here in this commit: 552ca48

Although, I'm not able to create a PR because I'm not a contributor on @dzdidi rust-teos repo I think?

@dzdidi would be great if you could let me if this works?

dzdidi · 2025-02-25T06:56:17Z

@sangbida added you as a collaborator

Update toolchain version Change the path to link cln

sangbida · 2025-02-25T13:16:52Z

Hey, I was able to get no errors here: dzdidi#2

Although, I can't tell if the failing test here ran on my branch? I can't see that it's passed.

Also wanted to know why this dependency upgrade is needed to add a watchtower client?

EDIT: All good, found this comment in the top level issue.

Add rust toolchain to cln-plugin.yml

mariocynicys

Thanks very much!

Gonna need one (or two) extra review iterations.

mariocynicys · 2025-02-28T18:00:32Z

.github/workflows/cln-plugin.yaml

      - name: Install bitcoind
        run: |
          wget https://bitcoincore.org/bin/bitcoin-core-${{ env.bitcoind_version }}/bitcoin-${{ env.bitcoind_version }}-x86_64-linux-gnu.tar.gz
          tar -xzf bitcoin-${{ env.bitcoind_version }}-x86_64-linux-gnu.tar.gz
-          ln -s $(pwd)/bitcoin-${{ env.bitcoind_version }}/bin/bitcoin* /usr/local/bin
+          sudo ln -s $(pwd)/bitcoin-${{ env.bitcoind_version }}/bin/bitcoin* /usr/local/bin


is this sudo necessary? why?

mariocynicys · 2025-02-28T19:09:34Z

teos/src/bitcoin_cli.rs

@@ -9,20 +9,19 @@
 * at your option.
 */

+use base64::{engine::general_purpose::URL_SAFE, Engine as _};


the trait Engine doesn't have to be exported as _.
use base64::{engine::general_purpose::URL_SAFE as BASE64, Engine};

mariocynicys · 2025-02-28T20:07:53Z

teos/src/bitcoin_cli.rs

    pub fn get_new_rpc_client(&self) -> std::io::Result<RpcClient> {
        let http_endpoint = HttpEndpoint::for_host(self.host.to_owned()).with_port(self.port);
-        let rpc_credentials = base64::encode(&format!("{}:{}", self.rpc_user, self.rpc_password));
-        RpcClient::new(&rpc_credentials, http_endpoint)
+        let rpc_credentials = URL_SAFE.encode(format!("{}:{}", self.rpc_user, self.rpc_password));
+        Ok(RpcClient::new(&rpc_credentials, http_endpoint))
    }


since this func is now infallible, we can return RpcClient right away, so without wrapping it in a result.

mariocynicys · 2025-02-28T20:18:20Z

teos-common/src/cryptography.rs

 pub fn sign(msg: &[u8], sk: &SecretKey) -> Result<String, Error> {
-    message_signing::sign(msg, sk)
+    Ok(message_signing::sign(msg, sk))
 }


we can unwrap this out of the result since it's infallible

mariocynicys · 2025-02-28T20:38:48Z

teos/src/carrier.rs

+        let vec = Vec::from_hex(TXID_HEX).unwrap();
+        let hash: bitcoin::hashes::sha256d::Hash = bitcoin::hashes::Hash::from_slice(&vec).unwrap();
+        let txid = Txid::from(hash);


we can Txid::from_str here instead.
4 more instances of these are below.

mariocynicys · 2025-02-28T21:38:40Z

watchtower-plugin/src/retrier.rs

@@ -1239,9 +1239,15 @@ mod tests {
                MAX_ELAPSED_TIME as f64 + MAX_RUN_TIME,
            ))
            .await;
-            let state = wt_client.lock().unwrap();
-            assert!(state.get_retrier_status(&tower_id).unwrap().is_idle());



is this consistently failing or just one or two times?

after the sleep above, we should ideally have the retrier idle and not wait until it is.

also could you share what system are u using for these tests? i find out that time/sleep tests fail a lot on macos systems for some reason.

mariocynicys · 2025-02-28T21:40:20Z

teos/src/test_utils.rs

+        // FIXME: replace this magic number with something meaningul
+        let bits = bitcoin::CompactTarget::from_consensus(553713663);


we can build a normal target and then convert it to a compact one.

mariocynicys · 2025-02-28T21:42:44Z

teos/src/test_utils.rs

-        let hashes = txdata.iter().map(|obj| obj.txid().as_hash());
-        let mut header = BlockHeader {
-            version: 0,
+        let hashes = txdata.iter().map(|obj| obj.compute_txid().to_raw_hash());


could u please replace obj with tx

teos/src/test_utils.rs

teos/src/tx_index.rs

dzdidi added 16 commits January 30, 2025 13:50

presumably updated common

6a3a153

Signed-off-by: dzdidi <[email protected]>

presumably updated teos/src/test_utils

91469b5

Signed-off-by: dzdidi <[email protected]>

presumably updated teos/src/tx_index

c3024d2

Signed-off-by: dzdidi <[email protected]>

drop bitcoin to 0.32.0 as is used by bitcoind-rpc

ffaabe4

Signed-off-by: dzdidi <[email protected]>

presumably updated teos/src/tx_carrier

198e191

Signed-off-by: dzdidi <[email protected]>

presumably getting there!

afd2469

Signed-off-by: dzdidi <[email protected]>

presumably getting even closer

7e59195

Signed-off-by: dzdidi <[email protected]>

build seem to be working

25e8727

Signed-off-by: dzdidi <[email protected]>

cargo fix

6396d61

Signed-off-by: dzdidi <[email protected]>

fix cryptography tests

fdd24c3

Signed-off-by: dzdidi <[email protected]>

fix tests for dbm

2bc0012

Signed-off-by: dzdidi <[email protected]>

fix tests pow header req

6c127be

Signed-off-by: dzdidi <[email protected]>

increase timeout for fixing test

af86b0e

Signed-off-by: dzdidi <[email protected]>

todo comment

c006f7d

Signed-off-by: dzdidi <[email protected]>

fix deprecation warnings

7f57880

Signed-off-by: dzdidi <[email protected]>

increase timeout for fixing test

6183904

Signed-off-by: dzdidi <[email protected]>

mariocynicys self-requested a review February 3, 2025 15:24

dzdidi mentioned this pull request Feb 3, 2025

Build a tower client for LDK #133

Open

2 tasks

fmt

82e0b09

Signed-off-by: dzdidi <[email protected]>

mariocynicys reviewed Feb 7, 2025

View reviewed changes

dzdidi added 2 commits February 10, 2025 14:56

fmt, clippy, review

d13e1ad

Signed-off-by: dzdidi <[email protected]>

add msrv and tool chain file

01b4e42

Signed-off-by: dzdidi <[email protected]>

dzdidi force-pushed the feat/deps-update branch from 8da267c to 01b4e42 Compare February 10, 2025 13:56

dzdidi added 3 commits February 10, 2025 15:57

revert const to original values, await for idling

de4cf76

Signed-off-by: dzdidi <[email protected]>

fmt

9d49657

Signed-off-by: dzdidi <[email protected]>

ci: cln v24.11.1

73f49ce

Signed-off-by: dzdidi <[email protected]>

Add rust toolchain to cln-plugin.yml

7cf1f1e

Update toolchain version Change the path to link cln

Merge pull request #2 from sangbidac/sangbida/fix-cln-ci-rust

cc7225d

Add rust toolchain to cln-plugin.yml

dzdidi mentioned this pull request Feb 25, 2025

Feat/teos ldk client dzdidi/rust-teos#3

Closed

dzdidi requested a review from mariocynicys February 26, 2025 09:31

mariocynicys reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/deps update #268

Feat/deps update #268

dzdidi commented Feb 3, 2025 •

edited

Loading

mariocynicys Feb 7, 2025

mariocynicys left a comment

mariocynicys Feb 7, 2025

mariocynicys Feb 7, 2025

mariocynicys Feb 7, 2025

mariocynicys Feb 7, 2025

mariocynicys Feb 7, 2025

mariocynicys Feb 7, 2025

mariocynicys Feb 7, 2025

dzdidi commented Feb 11, 2025

mariocynicys commented Feb 12, 2025

dzdidi commented Feb 19, 2025

mariocynicys commented Feb 20, 2025

dzdidi commented Feb 20, 2025

sangbida commented Feb 20, 2025 •

edited

Loading

sangbida commented Feb 22, 2025

mariocynicys commented Feb 22, 2025

sangbida commented Feb 22, 2025

sangbida commented Feb 25, 2025

dzdidi commented Feb 25, 2025

sangbida commented Feb 25, 2025 •

edited

Loading

mariocynicys left a comment

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

mariocynicys Feb 28, 2025

		dispute_txid: t.dispute_tx.compute_txid().to_raw_hash().encode(),
		penalty_txid: t.penalty_tx.compute_txid().to_raw_hash().encode(),

		// FIXME: replace this magic number with something meaningul
		let bits = bitcoin::CompactTarget::from_consensus(553713663);

Feat/deps update #268

Are you sure you want to change the base?

Feat/deps update #268

Conversation

dzdidi commented Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzdidi commented Feb 11, 2025

mariocynicys commented Feb 12, 2025

dzdidi commented Feb 19, 2025

mariocynicys commented Feb 20, 2025

dzdidi commented Feb 20, 2025

sangbida commented Feb 20, 2025 • edited Loading

sangbida commented Feb 22, 2025

mariocynicys commented Feb 22, 2025

sangbida commented Feb 22, 2025

sangbida commented Feb 25, 2025

dzdidi commented Feb 25, 2025

sangbida commented Feb 25, 2025 • edited Loading

mariocynicys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzdidi commented Feb 3, 2025 •

edited

Loading

sangbida commented Feb 20, 2025 •

edited

Loading

sangbida commented Feb 25, 2025 •

edited

Loading