feat: cookbook v1 #9

realeinherjar · 2023-09-24T11:42:57Z

Details

I want this cookbook to be tested automatically on CI, so the code will always run.
The only solution that worked for me was this workaround that is used in The Rust Rand Book.

.github/dependabot.yml

realeinherjar · 2023-09-24T11:43:37Z

.github/workflows/gh-pages.yml

+      - name: Deploy
+        uses: peaceiris/actions-gh-pages@v3
+        with:
+          github_token: ${{ secrets.GITHUB_TOKEN }}
+          publish_dir: ./cookbook/book


@tcharding this is totally wrong, I am adding as a placeholder now.
I'll update once I finish to add the content.
Feel free to comment, edit, contribute...

I'm very far from a GitHub actions expert, please take my lack of comments on the actions stuff as "I don't know enough to comment right now but I don't see anything too crazy"

I will update this soon with the correct code and any questions or if you feel something is odd let me know...

@tcharding the CI action for gh-pages deployment is ready to review. Should work now.

Double-checked once again and made slight tweaks. This should work now.

.github/workflows/test.yml

cookbook/src/03-parse.md

cookbook/src/02-p2p.md

cookbook/book.toml

tcharding · 2023-09-25T20:15:59Z

Ready for review, right? Perhaps squash it all down into a single commit, its going to be a punish to rebase otherwise. Since its the first major work on the cookbook its fine to be one enormous commit (at least I'm fine to review it as such). I got super excited when I ran mdbook serve and saw what you'd done, I've wanted to make progress on this for so long, thanks a million man!

(FTR I'm going to review it now as is still.)

apoelstra · 2023-09-25T20:17:14Z

+1 to squashing into one commit -- though if you wanna be really nice, you could maybe split it into sections and make each one its own commit, or something.

tcharding

Man this is so thorough! I did not review the taproot example, many of the comments on the segwit one will apply I'm guessing. I'll give you time to use/ignore my suggestions then review the taproot one.

Top work bro!

tcharding · 2023-09-25T20:18:11Z

.github/workflows/gh-pages.yml

+      - name: Deploy
+        uses: peaceiris/actions-gh-pages@v3
+        with:
+          github_token: ${{ secrets.GITHUB_TOKEN }}
+          publish_dir: ./cookbook/book


I'm very far from a GitHub actions expert, please take my lack of comments on the actions stuff as "I don't know enough to comment right now but I don't see anything too crazy"

.github/workflows/test.yml

README.md

cookbook/book.toml

cookbook/src/tx_segwit-v0.md

realeinherjar · 2023-09-25T22:26:38Z

+1 to squashing into one commit -- though if you wanna be really nice, you could maybe split it into sections and make each one its own commit, or something.

I thought you guys did "squash merge" here. But I can squash it sure.

We don't squash merge. We try to follow:

One patch per discreet change
Rebase after review adding review changes to original patch
Don't change code in a later patch that was changed in a previous one (so that reviewers don't comment on an early patch just to find its changed later on)

EDIT (tcharding): Ouch, I managed to edit your post yesterday instead of replying, sorry about that. FTR the answer here is written by me (tcharding). Sorry @realeinherjar, was not on purpose.

tcharding · 2023-09-25T22:57:09Z

+1 to squashing into one commit -- though if you wanna be really nice, you could maybe split it into sections and make each one its own commit, or something.

This PR is already bucket loads of work, speaking for myself I'd prefer you use your effort for real fixes to the cookbook rather than worrying about rebasing. This is example/docs code, its not as critical that we catch mistakes (which is a large part of what spitting it up helps achieve).

tcharding · 2023-09-25T23:00:40Z

We have "do not automatically run CI for first time contributors" configured, if you want to use CI runs during dev of this you could throw up a trivial PR doing something else and we can merge it for you.

realeinherjar · 2023-09-25T23:22:00Z

I see, I will fix something easy and then tag you as a reviewer.
But I am done for today, so tomorrow.

realeinherjar · 2023-09-26T10:57:30Z

I'll give you time to use/ignore my suggestions then review the taproot one.

I've added the segwit suggestions to the taproot one as well, since there was a lot of overlap.
It is ready to review.

cookbook/src/tx_segwit-v0.md

tcharding · 2023-09-26T20:37:53Z

I've added the segwit suggestions to the taproot one as well, since there was a lot of overlap. It is ready to review.

Great, will review - likely won't get to it today but I'll get to it. Thanks man.

@tcharding

…s should be enclose in ` cb0cfe4 fix(README): markdown URLs should be enclose in ` (Einherjar) Pull request description: Just a quick fix so I can run CI in #9. #9 (comment) Cc @tcharding ACKs for top commit: tcharding: ACK cb0cfe4 Tree-SHA512: 9ea55c3dc24d69b71e00a588968563a19ff767a2c0c8ae5917361675f887c9668b7575f7baa3221687c1211ac004d4b8be9b36dfe8cd839341fedcd7250bd552

tcharding · 2023-09-28T21:47:48Z

A quick rebase and force push and CI should run for you :)

realeinherjar · 2023-09-29T17:33:46Z

@tcharding note that the new GitHub CI for GH Pages replaces the bash script you've made (I'm deleting it).

Additionally, I think you've done everything necessary to host a GH Page in a custom domain.
I added a CNAME file following the instructions here: https://docs.github.com/en/pages/configuring-a-custom-domain-for-your-github-pages-site/managing-a-custom-domain-for-your-github-pages-site#about-custom-domain-configuration

Could you check them to see if we need to do anything else.
I would be glad to help in whatever you need.

cookbook/src/tx_segwit-v0.md

realeinherjar · 2023-09-30T10:32:29Z

Updated the examples from rust-bitcoin/rust-bitcoin#2095.
This is going to fail until we deal with the blockers in the PR description.

cookbook/src/tx_segwit-v0.md

tcharding · 2023-10-01T22:11:14Z

I'm not getting what you are trying to achieve with the blockers? Are you thinking this won't merge until after the next release?

realeinherjar · 2023-10-01T22:17:59Z

I'm not getting what you are trying to achieve with the blockers? Are you thinking this won't merge until after the next release?

I can revert back.
I thought you were worried that the example didn't added the public key as you've said:

Need to serialize the pubkey, see https://github.com/rust-bitcoin/rust-bitcoin/pull/2084/files

Since, this linked to the new Witness::p2wpkh constructor I thought we need to change the cookbook here.

But we can merge with the current rust-bitcoin version and update this later.

tcharding · 2023-10-01T23:07:10Z

oh sorry mate if I caused confusion. I meant to just link you to code that worked to show what was needed :) To clarify so we are both on the same page, I think the cookbook should depend on the latest released version and we should use the soon-to-merge code in examples to have the same thing depending on master.

realeinherjar · 2023-10-02T07:25:47Z

oh sorry mate if I caused confusion. I meant to just link you to code that worked to show what was needed :) To clarify so we are both on the same page, I think the cookbook should depend on the latest released version and we should use the soon-to-merge code in examples to have the same thing depending on master.

Ok, gotcha.
I will revert back the commits.
In the mean time, how do we fix the public key serialization in 0.30?

tcharding · 2023-10-03T01:29:37Z

In the mean time, how do we fix the public key serialization in 0.30?

The serialization is correct in 0.30 its just that the API is easy to mis-use. As an example the code tx_taproot.rs is correct because we use TapSighashType::Default, if a user inadvertantly copied this code then changed the sighash type the serialization would be wrong (missing the sighash type) - hence in https://github.com/rust-bitcoin/rust-bitcoin/pull/2095/files (and the new function in https://github.com/rust-bitcoin/rust-bitcoin/pull/2097/files#diff-ea912e7eb42db72b208cad947587783db67cfb40e77075e254420fbd774bf312) I used to_vec to get the correct behaviour irrespective of the sighash type - I think we should update this to do the same.

FTR one of the rust-bitcoin projects stated aims is to design APIs that are easy to use, hard to misuse, and don't require intimate knowledge of the bips or Bitcoin Core to use correctly. The Witness::push function is not currently conforming to these goals - now I write this I think we need to do more constructors and add docs to push saying "you should know exactly what you are doing if you push things onto the witness with this function".

tcharding · 2023-10-03T01:31:44Z

I knew this cookbook was a good idea and would shake things out.

tcharding · 2023-10-03T01:46:42Z

Just noticed that there is a commit with a mention in the git brief message - eb6c2eb fix: all @tcharding suggestions and fixes.

(Is this some sort of auto-generated patch by github?)

Just to clarify, how we tend to do things is:

each patch is a single conceptual change
after review, if changes are needed, rebase, fix the original patch, and force push
each patch needs a well formed description (describe why the change is needed then describe then change)

I hope my previous reviews did not lead you to the current git logs, I am trying to get better at reviewing :)

realeinherjar · 2023-10-03T10:44:08Z

FTR one of the rust-bitcoin projects stated aims is to design APIs that are easy to use, hard to misuse, and don't require intimate knowledge of the bips or Bitcoin Core to use correctly. The Witness::push function is not currently conforming to these goals - now I write this I think we need to do more constructors and add docs to push saying "you should know exactly what you are doing if you push things onto the witness with this function".

Totally agree with this.

Just noticed that there is a commit with a mention in the git brief message - eb6c2eb fix: all @tcharding suggestions and fixes.

I did it. But I've rebase everything to 2 commits :)

@tcharding this should be ready to review and merge now.

tcharding · 2023-10-03T23:42:37Z

In this repo I'd like to maintain the same merge policy as in the rest of rust-bitcoin org, that means:

Only merge PRs using the bitcoin merge script: https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/github-merge.py
(1) implies we use a merge commit when merging which leads to (3)
Discreet, well described commits, that do a single thing.

For (3) we want the format of the git message to be "describe why the change is needed" then "describe what the change is" (if trivial the git brief message can act as the "what").

So, can you please update the commit logs of the 2 patches? (And please don't use mentions (@superstar) in the git logs because doing so causes GitHub to spam the user with notifications.

sanket1729

Only reviewed segwit example. I think most comments are nit and can be addressed later if needed, but the script_code one should be addressed.

sanket1729 · 2023-10-04T06:06:33Z

cookbook/src/tx_segwit-v0.md

+This is the `cargo` commands that you need to run this example:
+
+```bash
+cargo add bitcoin --features "std, rand-std"


@tcharding, I think this is bug in rust-bitcoin. I would expect rand-std to include std.

rust-bitcoin/rust-bitcoin#2102

cookbook/src/tx_segwit-v0.md

sanket1729 · 2023-10-04T06:33:28Z

cookbook/src/tx_segwit-v0.md

+        .push(&pk.serialize());
+
+    // Print the transaction ready to broadcast
+    let tx = sighash_cache.into_transaction();


While sighash_cache::witness_mut this is a fine way to do sighash operations, I feel that it really non-intuitive. It is primarily to avoid allocations when iterating through tx inputs while creating witnesses.

I feel the below is better for a tutorial/cookbook.

// Update the witness wpkh stack: signature + public key. let pk = sk.public_key(&secp); let mut witness = &mut tx.input[0].witness; witness.push_bitcoin_signature( &sig.serialize_der(), EcdsaSighashType::All ); witness.push(&pk.serialize());

I've updated how we are mutating the witness in both segwit and taproot with your suggestion.
Can you check it now please?

cookbook/src/tx_segwit-v0.md

realeinherjar · 2023-10-04T15:44:14Z

@sanket1729 thanks for the review and suggestions.
I agreed with everything and incorporate them all.
Can you please check the witness updating procedure now in both segwit and taproot.

Here are the permalinks for easy visualization

Segwit:

www.rust-bitcoin.org/cookbook/src/tx_segwit-v0.md

Lines 208 to 229 in f15e680

    
           // Sign the unsigned transaction. 
        
           let mut sighash_cache = SighashCache::new(unsigned_tx); 
        
           let sighash = sighash_cache 
        
               .segwit_signature_hash(0, &script_code, DUMMY_UTXO_AMOUNT, EcdsaSighashType::All) 
        
               .expect("valid sighash"); 
        
           let msg = Message::from(sighash); 
        
           let sig = secp.sign_ecdsa(&msg, &sk); 
        
           // Convert into a transaction 
        
           let mut tx = sighash_cache.into_transaction(); 
        
           // Update the witness stack 
        
           let pk = sk.public_key(&secp); 
        
           let mut witness = &mut tx.input[0].witness; 
        
           witness.push_bitcoin_signature( 
        
               &sig.serialize_der(), 
        
               EcdsaSighashType::All 
        
           ); 
        
           witness.push(&pk.serialize()); 
        
           // Print the transaction ready to broadcast 
        
           println!("tx: {tx:?}");

Taproot

www.rust-bitcoin.org/cookbook/src/tx_taproot.md

Lines 212 to 241 in f15e680

    
           // The transaction we want to sign and broadcast. 
        
           let unsigned_tx = Transaction { 
        
               version: 2, 
        
               lock_time: absolute::LockTime::ZERO, 
        
               input: vec![input], 
        
               output: vec![spend, change], 
        
           }; 
        
           // Create the prevouts 
        
           let prevouts = vec![dummy_utxo]; 
        
           let prevouts = Prevouts::All(&prevouts); 
        
           // Sign the unsigned transaction. 
        
           let mut sighash_cache = SighashCache::new(unsigned_tx); 
        
           let sighash = sighash_cache 
        
               .taproot_signature_hash(0, &prevouts, None, None, TapSighashType::Default) 
        
               .expect("valid sighash"); 
        
           let tweaked: TweakedKeyPair = keypair.tap_tweak(&secp, None); 
        
           let msg = Message::from(sighash); 
        
           let sig = secp.sign_schnorr(&msg, &tweaked.to_inner()); 
        
           // Convert into a transaction 
        
           let mut tx = sighash_cache.into_transaction(); 
        
           // Update the witness stack 
        
           let mut witness = &mut tx.input[0].witness; 
        
           witness.push(sig.as_ref()); 
        
           // Print the transaction ready to broadcast 
        
           println!("tx: {tx:?}");

tcharding

ACK 2993048

tcharding · 2023-10-04T23:35:40Z

I've acked but can you re-write the commit logs before this is considered for merge please. Also I'm not totally confident in my ability to review the github actions stuff, especially the deployment, but if it works its way better than the script I had hacked up. We will find out soon enough when we merge, YOLO!

tcharding · 2023-10-04T23:36:15Z

FTR I served the cookbook locally and it looks awesome!

sanket1729

ACK 2993048. Just reviewed the examples for correctness and verified that I can serve the files locally.

I am not too familiar with github actions stuff going on here, but this is good to go from my side.

Cookbook on how to sign segwit and taproot transactions Additionally: - Convert `build.sh` to a GitHub Action - Automated CI in GitHub Actions that test all the code in the cookbook - Automated scheduled CI in GitHub Actions that check all markdown files for broken links - Dependabot to update (weekly) - GitHub Actions - Test dependencies - `justfile` to easy run the build and test with `just`

realeinherjar · 2023-10-05T08:48:40Z

Thanks for the reviews and for dedicating time to help in the making of this content.

I am 99% certain on the GitHub Pages deployment.
However, once we fix this we don't need to touch it or run shell scripts again to build a new version of the site.
If anything goes wrong with the site I'll fix it straight away.

@tcharding I've squashed all to a single commit and changed the commit message.
See if you want me to change anything else.

tcharding

ACK cee4953

tcharding · 2023-10-06T01:53:37Z

Looks good, lets go!

tcharding · 2023-10-06T01:55:52Z

I checked the diff since you reviewed @sanket1729 and its only changes to the github actions etc., no code changes to the examlpes.

I'll let this sit over the weekend then merge it unless anyone wants more time to look.

sanket1729 · 2023-10-06T08:29:35Z

utACK cee4953. Go ahead

tcharding · 2023-10-07T00:35:54Z

Doesn't look like this has been deployed. I cleared my cache and checked the site. Did not investigate further (its the weekend here).

realeinherjar · 2023-10-07T08:06:05Z

Ok I was asleep. Looks like some Hugo error. I will debug it and submit a fix in the next hours.
That came out of the blue.

realeinherjar · 2023-10-07T08:37:07Z

Fix in #14 @tcharding
(Sorry about that)

tcharding · 2023-10-07T22:51:12Z

No sweat, thanks for the fix. Was cool to wake up and have this fixed already.

realeinherjar · 2023-10-08T08:03:14Z

@tcharding the website builds ok now.
But we need someone with maintainer access (needs to be able to set the repo settings) to activate GitHub pages.

The instructions are here: https://docs.github.com/en/pages/getting-started-with-github-pages/configuring-a-publishing-source-for-your-github-pages-site#publishing-from-a-branch
We need to "Publish from a branch" and the defaults gh-pages and / (root) will suffice.

Finally, since it is a custom domain you might need to do some tweaks explained here: https://docs.github.com/en/pages/configuring-a-custom-domain-for-your-github-pages-site/about-custom-domains-and-github-pages

EDIT: If you are struggling with this, let me know. We can jump in a quick call and I can guide you through if necessary...

tcharding · 2023-10-08T21:54:00Z

Oh, this is my fault for not remembering how I set this up. How I had the build script and the repos set up doesn't work with the actions stuff we just merged. I had it so that this repo was just the source code and the actual site, once built, was pushed to https://github.com/rust-bitcoin/rust-bitcoin.github.io/tree/master. That repo has the custom domain etc. configured already. The reasoning behind this split was so that the source code was not entangled with the hosting service (currently github pages) so that if we wanted to move to a different host it was trivial to do. Other maintainers, if I remember correctly, didn't really see the need for this so I guess we could just delete the other repo and configure github pages from this repo. I'm fasting at the moment and in general I really hate doing any sort of web development, but I'll come back to this once I'me eating again.

realeinherjar · 2023-10-08T22:01:54Z

As always if you need an extra pair of hands let me know...
I agree that https://github.com/rust-bitcoin/rust-bitcoin.github.io should be deleted and replaced by https://github.com/rust-bitcoin/www.rust-bitcoin.org.

realeinherjar commented Sep 24, 2023

View reviewed changes

.github/dependabot.yml Outdated Show resolved Hide resolved

realeinherjar commented Sep 24, 2023

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

realeinherjar changed the title ~~Cookbook~~ feat: cookbook v1 Sep 24, 2023

realeinherjar commented Sep 24, 2023

View reviewed changes

cookbook/src/03-parse.md Outdated Show resolved Hide resolved

realeinherjar commented Sep 24, 2023

View reviewed changes

cookbook/src/02-p2p.md Outdated Show resolved Hide resolved

tcharding reviewed Sep 24, 2023

View reviewed changes

cookbook/book.toml Show resolved Hide resolved

realeinherjar marked this pull request as ready for review September 25, 2023 16:52

realeinherjar requested a review from tcharding September 25, 2023 16:52

tcharding reviewed Sep 25, 2023

View reviewed changes

realeinherjar mentioned this pull request Sep 26, 2023

fix(README): markdown URLs should be enclose in ` #12

Merged

tcharding reviewed Sep 26, 2023

View reviewed changes

cookbook/src/tx_segwit-v0.md Outdated Show resolved Hide resolved

tcharding mentioned this pull request Sep 29, 2023

bitcoin: Add signing examples rust-bitcoin/rust-bitcoin#2095

Merged

realeinherjar commented Sep 29, 2023

View reviewed changes

cookbook/src/tx_segwit-v0.md Outdated Show resolved Hide resolved

tcharding reviewed Oct 1, 2023

View reviewed changes

cookbook/src/tx_segwit-v0.md Outdated Show resolved Hide resolved

tcharding mentioned this pull request Oct 3, 2023

Generated HTML does not include the logo #6

Closed

sanket1729 reviewed Oct 4, 2023

View reviewed changes

realeinherjar mentioned this pull request Oct 4, 2023

rand-std should enable std rust-bitcoin/rust-bitcoin#2102

Merged

tcharding approved these changes Oct 4, 2023

View reviewed changes

sanket1729 approved these changes Oct 5, 2023

View reviewed changes

tcharding approved these changes Oct 6, 2023

View reviewed changes

tcharding merged commit 4d89d43 into rust-bitcoin:master Oct 6, 2023
1 check passed

realeinherjar deleted the einherjar/cookbook-feat branch October 7, 2023 08:06

realeinherjar mentioned this pull request Oct 12, 2023

Fix site deployment #17

Closed

feat: cookbook v1 #9

feat: cookbook v1 #9

Conversation

realeinherjar commented Sep 24, 2023 • edited

Details

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 25, 2023 • edited

apoelstra commented Sep 25, 2023

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

realeinherjar commented Sep 25, 2023 • edited by tcharding

tcharding commented Sep 25, 2023 • edited

tcharding commented Sep 25, 2023

realeinherjar commented Sep 25, 2023

realeinherjar commented Sep 26, 2023

tcharding commented Sep 26, 2023 • edited

tcharding commented Sep 28, 2023

realeinherjar commented Sep 29, 2023

realeinherjar commented Sep 30, 2023

tcharding commented Oct 1, 2023

realeinherjar commented Oct 1, 2023

tcharding commented Oct 1, 2023

realeinherjar commented Oct 2, 2023

tcharding commented Oct 3, 2023 • edited

tcharding commented Oct 3, 2023

tcharding commented Oct 3, 2023

realeinherjar commented Oct 3, 2023

tcharding commented Oct 3, 2023

sanket1729 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

realeinherjar commented Oct 4, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Oct 4, 2023

tcharding commented Oct 4, 2023 • edited

sanket1729 left a comment

Choose a reason for hiding this comment

realeinherjar commented Oct 5, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Oct 6, 2023

tcharding commented Oct 6, 2023

sanket1729 commented Oct 6, 2023

tcharding commented Oct 7, 2023 • edited

realeinherjar commented Oct 7, 2023

realeinherjar commented Oct 7, 2023

tcharding commented Oct 7, 2023

realeinherjar commented Oct 8, 2023 • edited

tcharding commented Oct 8, 2023

realeinherjar commented Oct 8, 2023

realeinherjar commented Sep 24, 2023 •

edited

tcharding commented Sep 25, 2023 •

edited

realeinherjar commented Sep 25, 2023 •

edited by tcharding

tcharding commented Sep 25, 2023 •

edited

tcharding commented Sep 26, 2023 •

edited

tcharding commented Oct 3, 2023 •

edited

tcharding commented Oct 4, 2023 •

edited

tcharding commented Oct 7, 2023 •

edited

realeinherjar commented Oct 8, 2023 •

edited