Add `submitpackage` support #23

tnull · 2024-08-30T12:22:02Z

~~Based on #22.~~

Rebased after #22 landed. Should be good for review.

tcharding

Looks good so far.

tcharding · 2024-09-01T21:57:20Z

json/src/v28/raw_transactions.rs

+    /// whose fees and vsizes are included in effective-feerate.
+    #[serde(rename = "effective-includes")]
+    pub effective_includes: Vec<Wtxid>,
+}


These structs look good, we will need to also add similar structs in json/src/model and also into_model() functions on each of them here in this module.

Ah, I think on the first attempt I didn't quite get when code would fully live in one of the vXX modules and when it would go in model. IIUC, Is the idea that everthing should be duplicated in model and the model is fed with the respective implementations based on the features?

In model the structs are fully typed i.e., in the respective json version module we use standard language types (String, u32) and in model we use bitcoin types (BlockHash, Version). The into_model function is to do the conversion. The hope is that then the model structs will be somewhat universal across all the Core versions without having a bunch of conditional logic in them. We will only know if this works after writing a bunch more code though. I did iterate quite a few times already to get to this design, so I am hopeful.

Hmm, I think I still don't fully get why the model is necessary. Why can't we just use serde's with attribute for the fields that need to be de/serialized differently from their Rust type?
It would allow to avoid a lot of redundant boilerplate, at least if I understand the problem at hand correctly?

For example, see these definitions: https://github.com/lightningdevkit/lightning-liquidity/blob/8dd8a1cdf52d5913ff18d9677c152c521b22c4cc/src/lsps0/ser.rs#L603-L678 which are used here: https://github.com/lightningdevkit/lightning-liquidity/blob/8dd8a1cdf52d5913ff18d9677c152c521b22c4cc/src/lsps2/msgs.rs#L85-L105

The problem the model module is trying to solve is multiple versions of Core returning different things for the same JSONRPC method. This led, in rust-bitcoincore-rpc, to loads of logic inside the types crate to handle the differences, this was scattered all over the place making it hard to know exactly what was supported and what was not. The idea in this repo is to make the version specific types dumb as possible, then the model types are, if possible, some representation of the data returned by all supported Core versions. Each version specific type then implements into_model (if possible). Users are free to not use the model types at all if they only want standard typed fields and not the rust-bitcoin typed fields in model. Does that make sense?

I don't think there is a good solution to Core's JSONRPC API returning different data for different versions

Mhh, I see the issue for the raw JSON types, but want to note we could somewhat mitigate the issue in the client implementation by checking the version after connection (e.g., through getnetworkinfo), and switch to the matching types accordingly? In any case, I'll just proceed for now.

Yes in theory, but someone somewhere said that some ops guys disable that RPC method for some security reason that I don't remember so their is no reliable way to get the version. The fella shortly afterwards did a PR to Core to add a new method to get just the version. I can dig it out if you want but since we want to support old versions it doesn't help us much.

I can dig it out if you want but since we want to support old versions it doesn't help us much.

Since it came up once or twice since you wrote this, I'd actually be interested to hear how and why they do this. Do you think you can still find it?

Right now I have no clue, but I can keep you in mind and message you if/when I stumble upon it.

These structs look good

Bother, sorry I did not notice the rust-bitcoin types here last month when I reviewed. I bet I caused you to be confused about the purpose into_model this last week when you implemented it.

tcharding · 2024-09-01T21:58:18Z

Woops, no need to run CI we know its going to fail because there is no binary to pull down.

tcharding · 2024-09-01T21:59:27Z

How hard was it to work out how to patch this repo @tnull? I realise its a bit unusual, from your experience do you think I'm correct in thinking that there is no point asking more junior devs to work on this repo because its a little too strange?

tnull · 2024-09-02T08:00:32Z

How hard was it to work out how to patch this repo @tnull? I realise its a bit unusual, from your experience do you think I'm correct in thinking that there is no point asking more junior devs to work on this repo because its a little too strange?

Given there are no docs describing the ideas behind the project layout (w.r.t. features etc) it took a while to get the hang of it. I think if it would be clearly documented how everything's supposed to work it shouldn't be too hard to contribute though, even for more junior devs.

tcharding · 2024-09-02T21:01:53Z

No sweat, I actually added a wiki doc yesterday before reading your comment above but we can do even better. Thanks man.

tnull · 2024-11-07T13:13:17Z

I now rebased this after #22 landed, fixed some minor things and added a really basic test for most of the introduced API types (we don't actually test package submission, but submission of two already-known transactions).

Should be good for review (cc @tcharding)

tcharding · 2024-11-09T00:48:06Z

Thanks for the work man, I didn't review closely but a few quick comments:

Can you do this as a single patch please since its a discreet change
Can you remove the commented out docs from Bitocin Core in json/src/v28/raw_transactions.rs
We either need an issue saying "Implement into_model for SubmitPackage" or we should implement it in this PR

json/src/v28/raw_transactions.rs

tnull · 2024-11-11T09:16:22Z

Thanks for the work man, I didn't review closely but a few quick comments:
* Can you do this as a single patch please since its a discreet change

If you prefer, now squashed the commits and added further fixups.

* Can you remove the commented out docs from Bitocin Core in `json/src/v28/raw_transactions.rs`

Hmm, I intentionally didn't include the bitcoind output as a doc-comment since a) it's slightly wrong/misleading in the Rust context and b) is redundant to the actual field docs.

* We either need an issue saying "Implement `into_model` for `SubmitPackage`" or we should implement it in this PR

Done

tcharding · 2024-11-12T02:15:11Z

Hmm, I intentionally didn't include the bitcoind output as a doc-comment since a) it's slightly wrong/misleading in the Rust context and b) is redundant to the actual field docs.

Yeah I agree with this, I mean we don't want it as a code comment either simply because if we do this everywhere there will be too much noise IMO, or is this specific method special on some way I'm missing that means the comment is needed?

tcharding

Holla if the concept between model vs v28/raw_transaction is not clear man, I can try explain it a different way if so.

json/src/model/raw_transactions.rs

json/src/v28/raw_transactions.rs

tcharding · 2024-11-12T03:22:11Z

json/src/v28/raw_transactions.rs

+    /// whose fees and vsizes are included in effective-feerate.
+    #[serde(rename = "effective-includes")]
+    pub effective_includes: Vec<Wtxid>,
+}


These structs look good

Bother, sorry I did not notice the rust-bitcoin types here last month when I reviewed. I bet I caused you to be confused about the purpose into_model this last week when you implemented it.

tnull · 2024-11-12T09:32:27Z

Holla if the concept between model vs v28/raw_transaction is not clear man, I can try explain it a different way if so.

No, I think I get it overall. I have to say though that it makes for a pretty unergonomic API, since it requires doing call().unwrap().into_model().unwrap() everywhere and users might not expect that they are required to manually initiate the actual parsing in the API essentially.

tnull · 2024-11-12T10:32:55Z

Now made the requested changes.

tcharding · 2024-11-12T23:21:19Z

I have to say though that it makes for a pretty unergonomic API

I tend to agree with you. Its the best API I could come up with that solves the problem. I'm open to suggestions but the idea to split the model types from the actual types returned by the JSONRPC methods is core to the design of the crate and I still believe that it is the correct decision.

tcharding · 2024-11-12T23:22:25Z

If you grep for cl. in this file https://github.com/rust-bitcoin/rust-miniscript/blob/master/bitcoind-tests/tests/test_cpp.rs you can see that how I used the Client. I updated these tests while designing the API and this was the best that I could come up with.

tcharding

Thanks man, I realize that this crate is a bit odd and probably annoying to work on so thanks for doing so. I had a few more nits but I didn't want to be too anal about it, I can come back and do a follow up if that doesn't annoy you or I can post all the nits if you like. Because the crate is basically all boilerplate I'm trying to be really uniform.

Thanks again

tcharding · 2024-11-12T23:36:34Z

json/src/model/raw_transactions.rs

+
+/// Error when converting a `SubmitPackageTxResultFees` type into the model type.
+#[derive(Debug)]
+pub enum SubmitPackageError {


This error type needs to go over in v28/raw_transactions.rs (the conversion is coupled with the into_model function). Also the error needs a few more things implementing, see v17/blockchain.rs for an example. FTR the v17 stuff is in the best state because its the part of the crate I've been working on lately, if something is non-uniform favour the stuff in v17.

Huh, well that's surprising to me. In fact I started the error type in v28 but then realized it is / needs to be part of the stable model conversion API, and hence shouldn't / can't be version-specific, no?

tcharding · 2024-11-12T23:42:16Z

json/src/v28/raw_transactions.rs

+        let vsize = if let Some(vsize) = self.vsize {
+            Some(vsize.try_into().map_err(|_| SubmitPackageError::Vsize)?)


Does this definitely need to be a usize, a u32 would be ok, right? Then we can use the to_u32 function and put a NumericError variant in the SubmitPackageError (again see the v17 blockchain stuff for an example)
Here is an example

let stripped_size = self.stripped_size.map(|size| crate::to_u32(size, "stripped_size")).transpose()?;

Does this definitely need to be a usize, a u32 would be ok, right?

No, as it's a usize in bitcoin::Transaction? https://docs.rs/bitcoin/latest/bitcoin/blockdata/transaction/struct.Transaction.html#method.vsize

Doesn't make a lot of sense to me to first convert from i32 to u32 to then eventually convert it to usize (or have the user convert it) anyways?

tnull · 2024-11-14T08:38:19Z

If you grep for cl. in this file https://github.com/rust-bitcoin/rust-miniscript/blob/master/bitcoind-tests/tests/test_cpp.rs you can see that how I used the Client. I updated these tests while designing the API and this was the best that I could come up with.

Mhh, seems this 'always call into_model' logic should live in the client crate eventually? And possibly it could adhere to some more standard Rust patterns, e.g., impl TryFrom<X> for Y?

I had a few more nits but I didn't want to be too anal about it, I can come back and do a follow up if that doesn't annoy you or I can post all the nits if you like.

Please go ahead!

tcharding · 2024-11-25T21:56:12Z

Hey @tnull sorry to be a pain in the arse but I've moved this repo to a new name. Its now called corepc. See #48 and #51 for context.

Are you willing to re-do this? If not I'll get to it eventually. I'm focused on the earlier versions still though ATM. Thanks man.

(I moved all issues to the new repo but one cannot move PRs its seems.)

tnull · 2024-11-26T09:20:14Z

Are you willing to re-do this?

I mean, sure. Is there anything else I should include besides just reopening the PR over there?

tnull marked this pull request as draft August 30, 2024 12:22

tcharding reviewed Sep 1, 2024

View reviewed changes

tnull force-pushed the 2024-08-add-submitpackage branch from 3a3aa71 to a290eb4 Compare September 2, 2024 08:00

tnull force-pushed the 2024-08-add-submitpackage branch 2 times, most recently from 7e61405 to 4230085 Compare November 7, 2024 13:11

tnull changed the title ~~WIP: Add submitpackage support~~ Add submitpackage support Nov 7, 2024

tnull marked this pull request as ready for review November 7, 2024 13:11

tnull requested a review from tcharding November 7, 2024 13:11

tcharding reviewed Nov 9, 2024

View reviewed changes

json/src/v28/raw_transactions.rs Outdated Show resolved Hide resolved

Add submitpackage RPC support

bd6863c

f Include into_model implementations

7041357

tnull force-pushed the 2024-08-add-submitpackage branch from 4230085 to 7041357 Compare November 11, 2024 09:16

tcharding reviewed Nov 12, 2024

View reviewed changes

tnull added 3 commits November 12, 2024 10:36

f Drop serde stuff from model

9491250

f Don't use rust-bitcoin types in non-model

0517507

f Drop submitpackage RPC example output comment

156be9c

tcharding reviewed Nov 12, 2024

View reviewed changes

This was referenced Nov 26, 2024

Add support for submitpackage rust-bitcoin/corepc#7

Closed

v28: Add support for submitpackage rust-bitcoin/corepc#14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `submitpackage` support #23

Add `submitpackage` support #23

tnull commented Aug 30, 2024 •

edited

Loading

tcharding left a comment

tcharding Sep 1, 2024

tnull Sep 2, 2024

tcharding Sep 2, 2024

tnull Sep 3, 2024

tcharding Sep 3, 2024

tnull Sep 5, 2024

tcharding Sep 5, 2024

tnull Oct 10, 2024

tcharding Oct 13, 2024

tcharding Nov 12, 2024

tcharding commented Sep 1, 2024

tcharding commented Sep 1, 2024

tnull commented Sep 2, 2024

tcharding commented Sep 2, 2024

tnull commented Nov 7, 2024

tcharding commented Nov 9, 2024

tnull commented Nov 11, 2024

tcharding commented Nov 12, 2024

tcharding left a comment

tcharding Nov 12, 2024

tnull commented Nov 12, 2024 •

edited

Loading

tnull commented Nov 12, 2024

tcharding commented Nov 12, 2024

tcharding commented Nov 12, 2024 •

edited

Loading

tcharding left a comment •

edited

Loading

tcharding Nov 12, 2024

tnull Nov 14, 2024

tcharding Nov 12, 2024

tnull Nov 14, 2024

tnull commented Nov 14, 2024

tcharding commented Nov 25, 2024

tnull commented Nov 26, 2024

		let vsize = if let Some(vsize) = self.vsize {
		Some(vsize.try_into().map_err(\|_\| SubmitPackageError::Vsize)?)

Add submitpackage support #23

Are you sure you want to change the base?

Add submitpackage support #23

Conversation

tnull commented Aug 30, 2024 • edited Loading

tcharding 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Sep 1, 2024

tcharding commented Sep 1, 2024

tnull commented Sep 2, 2024

tcharding commented Sep 2, 2024

tnull commented Nov 7, 2024

tcharding commented Nov 9, 2024

tnull commented Nov 11, 2024

tcharding commented Nov 12, 2024

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Nov 12, 2024 • edited Loading

tnull commented Nov 12, 2024

tcharding commented Nov 12, 2024

tcharding commented Nov 12, 2024 • edited Loading

tcharding left a comment • edited Loading

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

tnull commented Nov 14, 2024

tcharding commented Nov 25, 2024

tnull commented Nov 26, 2024

Add `submitpackage` support #23

Add `submitpackage` support #23

tnull commented Aug 30, 2024 •

edited

Loading

tnull commented Nov 12, 2024 •

edited

Loading

tcharding commented Nov 12, 2024 •

edited

Loading

tcharding left a comment •

edited

Loading