Spend your coin remake by andrasfuchs · Pull Request #104 · ProgrammingBlockchain/ProgrammingBlockchain

andrasfuchs · 2018-01-04T17:35:37Z

Changed many things including the followings:

changed the tutorial to make sure that the reader uses the TestNet first
fixed all the links the TestNet blockchain explorer
re-did the tutorial myself, and replaced the addresses your more recent ones
adjusted the bitcoin amounts in the transactions to reflect the price/fee changes
also changed the amount to make it easier to follow the logic
fixed the link title for the CodeProject articles
created a new image to reflect the changes of BTC amounts
added a small section explaining BTC units

Changed many things including the followings: * changed the tutorial to make sure that they use the TestNet first * fixed all the links the TestNet blockchain explorer * re-did the tutorial myself, and replaced the addresses your more recent ones * adjusted the bitcoin amounts in the transactions to reflect the price/fee changes * also changed the amount to make it easier to follow the logic * fixed the link title for the CodeProject articles * created a new image to reflect the changes of BTC amounts

Changed wording according to issue ProgrammingBlockchain#93

nopara73 · 2018-01-04T21:46:38Z

+What happens to the remaining **70 &micro;BTC**? This is the _miner fee_.  
+The miner fee incentivizes the miners to add this transaction into their next block. The higher the miner fee the more motivated the miner is to include your transaction in the next block, meaning that your transaction will be confirmed faster. If you set the miner fee to zero, your transaction might never be confirmed.

 ![](../assets/SpendTx.png)


I don't like the new illustration. There is no consensus on uBTC. Plus the miner fee was previously illustrated by having a smaller coin that goes for the miner.

Hmm, interesting, I thought that the different sizes for the icons were just a mistake, it looked funny and pixelarated to me. Alright, I can change it back to various sizes. The amount of the miner fee was missing from the previous version, that wasn't intentional I suppose?

It was intentional, denoting "it's so small, it doesn't matter." Unfortunately it's not the case anymore :(

Agree with @nopara73 about the size, I think you could use bits though.

Ok, so I'll change the size of the coins back and I'll use bits instead on uBTC.

nopara73 · 2018-01-04T21:48:17Z

 So now that you know what a **bitcoin address**, a **ScriptPubKey**, a **private key** and a **miner** are, you will make your first **transaction** by hand.

-As you proceed through this lesson you will add code line by line as it is presented to build a method that will leave feedback for the book in a Twitter style message.
+As you proceed through this lesson you will add code line by line as it is presented to build a method that will leave feedback for the book in a Twitter style message. I _highly recommend_ you to follow the instructions first on the TestNet and after do them again on the Main Bitcoin network.


Mainnet practice is more memorable. If you have money on the line you are more inclined to pay attention.

I agree, but all the samples in the chapter were and are on TestNet, so to avoid confusion I wanted to make clear that it is better to start with the testnet and then go on and repeat them on mainnet.

On my side I am OK with switching to testnet. The first chapter is still using Mainnet which is enough to "feel" your money on the line. Also now, with the high fees, such experience everywhere could be very costly. :)

Ah wait! This is the first chapter.

nopara73 · 2018-01-04T21:48:54Z


 ```cs
-var network = Network.Main;
+// Replace this with Network.Main to do this on Bitcoin MainNet


Same comment as above.

nopara73 · 2018-01-04T21:52:47Z

 ```

-Note the **bitcoinPrivateKey** and the **address**, send some coins there and note the transaction id \(you can find it, probably, in your wallet software or with a blockexplorer, like [blockchain.info](http://blockchain.info/)\).
+Note that we use the TestNet first, but you will probably do this on the MainNet as well, so you are going to spend real money! **Be careful!** In any case, write down the **bitcoinPrivateKey** and the address! Send a few dollars of coins there and save the transaction ID \(you can find it in your wallet software or with a blockexplorer, like [blockchain.info](http://blockchain.info/) for MainNet or [blockcypher.com](https://live.blockcypher.com) for both MainNet and TestNet).


Same comment about mainnet vs testnet as above. Also I believe the "be careful, pay attention" is unnecessary hand holding. If they got to this point they must be aware of the risks. Also personally I prefer recommending SmartBit, instead of blockchain.info or blockcypher, since it's written with NBitcoin, and its API is vastly superior to every other bc explorer api.

Alright, I'll remove the be careful line, I thought that it's better to err on the safe side, but it's probably unnecessary.
Regarding SmartBit: I didn't know that you prefer that, I just found a comment (#1) yesterday after created this pull request. I'll change all the links to that, no problem.
Please note, that it is terribly slow from Europe at the moment, I hope it's much better from overseas.

+1000 for using smartbit instead on Blockchain info!

nopara73 · 2018-01-04T21:54:18Z


-Console.WriteLine(bitcoinPrivateKey); // cSZjE4aJNPpBtU6xvJ6J4iBzDgTmzTjbq8w2kqnYvAprBCyTsG4x
-Console.WriteLine(address); // mzK6Jy5mer3ABBxfHdcxXEChsn3mkv8qJv
+Console.WriteLine(bitcoinPrivateKey);


I believe, leaving a comment on the expected output is a good practice, since most people will not follow the book while coding.

Alright, I can agree on this one too. The only reason I thought that it is not so important here, because the user will have different values when he is doing the exercise (because of the different private key he generated for himself).
I'll put it back.

nopara73 · 2018-01-04T22:03:52Z

+TxOut changeTxOut = new TxOut()
 {
-    Value = new Money((decimal)0.4999, MoneyUnit.BTC),
+    Value = new Money(530, MoneyUnit.Bit),


Same comment as above.

nopara73 · 2018-01-04T22:05:17Z

+ * the currently advised mining fee,
+ * you may consider to increase or decrease it.
+ */
+var minerFee = new Money(70, MoneyUnit.Bit);


Same comment as above.

nopara73 · 2018-01-04T22:06:13Z


-Now add your feedback! This must be less than 40 bytes, or it will crash the application.  
-This feedback, along with your transaction will appear \(after your transaction is confirmed\) in the [Hall of The Makers](http://n.bitcoin.ninja/).
+Now add your personal feedback! This must be less than 80 bytes or your transaction will be rejected.  


Isn't it maximum 80 bytes?

Let's pay attention to the details :)

You mean the right wording would be "less then or equal to"?

nopara73 · 2018-01-04T22:08:50Z

 ```json
 {
-  "hash": "b7803df4b90fd615532bcbdb3b63eb1af5a2e4ae36f29a6fbf9f57d0a1842e0a",
+  "hash": "579ae72016c02691a9e47258d65705575d0a6a5f265b137c67ef6be7b2a35659",


Please pay attention for these to be consistent. I'm kind of confused during this review so I'm not sure I requested modification in the previous lines or not.

That's again different because of the new transaction with the new values. It is consistent with the data in the block explorer.

nopara73 · 2018-01-04T22:09:42Z

-Let's check out the **hash** of **prev\_out** in a blockexplorer: [http://tbtc.blockr.io/api/v1/tx/info/e44587cf08b4f03b0e8b4ae7562217796ec47b8c91666681d71329b764add2e3](http://tbtc.blockr.io/api/v1/tx/info/e44587cf08b4f03b0e8b4ae7562217796ec47b8c91666681d71329b764add2e3)  
-In **prev\_out** **n** is 1. Since we are indexing from 0, this means I want to spend the second output of the transaction.  
-In the blockexplorer we can see the corresponding address is `mzK6Jy5mer3ABBxfHdcxXEChsn3mkv8qJv` and I can get the scriptSig from the address like this:
+Let's check out the **hash** of **prev\_out** in a TestNet blockexplorer: [prev\_out tx details](https://live.blockcypher.com/btc-testnet/tx/0acb6e97b228b838049ffbd528571c5e3edd003f0ca8ef61940166dc3081b78a/).  You can see that 0.001 BTC, so 1'000 &micro;BTC was transferred to our address.


As I said SmartBit would be nicer.

Ok, no problem.

NicolasDorier · 2018-01-05T09:10:33Z


 ```cs
-var hallOfTheMakersAddress = BitcoinAddress.Create("mzp4No5cmCXjZUpf112B1XWsvWBfws5bbB");
+var hallOfTheMakersAddress = BitcoinAddress.Create("mzp4No5cmCXjZUpf112B1XWsvWBfws5bbB", Network.TestNet);


NBitcoin is intelligent enough to know it is testnet without hint, but it is better practice to specify it.

NicolasDorier · 2018-01-05T09:21:56Z

Ok so in overall I think:

Introducing Satoshi, BTC and Bits makes sense
Using Bits for the mainnet exercise makes sense
For testnet stuff BTC is better than Bit,
For mainnet stuff I have small preferences to Bit, even if not used to it yet,
The book should have the first exercise on mainnet (hello world), the rest can be on testnet. (We can provide instruction for people trying to use testnet though)

* Removed the "be careful" expression * Replaced all blockchain explorer links with SmartBit * Added back the console output comments * Fixed a few typos * Rewrote the unit description section * Changed microBTC to bits * Modified the SpendTx.png picture

andrasfuchs · 2018-01-05T13:20:01Z

I hope it got better, let me know what other changes you would prefer.

NicolasDorier · 2018-01-09T04:07:25Z

@nopara73 seems good to you?

nopara73 · 2018-01-09T04:23:04Z

@NicolasDorier Maybe. It's a little confusing. I let you decide.
@andrasfuchs It'd be better in the future to avoid monster commits. Usually separate issues should be created for every modification, like this:

Issue 1: Fix typos.
Issue 2: Change BTC denomination to bits.
Issue 3: Change mainnet to testnet.

Anyway, if you really want to go with bits then so be it, I'm strongly against it, but I respect the consensus.

#104 (comment)

andrasfuchs · 2018-01-09T07:29:05Z

BTC is fine with me too, I just need to spare a little time to change it back. You guys are the experts, so now that you have an agreement on it and you think bits are confusing then I remove it, no problem at all.

Would you like me to remove the units section that introduces bits and satoshis as well?

@nopara73 After we started to talk about this PR I realized that I made a mistake by not splitting it into a few smaller ones, so I will definitely take your advice next time :)

I'll do the changes in the next few days as soon as I have a time for it. I just need a quick reply from you guys, if you want to keep the unit introduction section. Thanks!

nopara73 · 2018-01-09T15:18:48Z

@andrasfuchs As I said I am strongly against it. But if Nicolas insist to bits, so be it, I don't think it's that important to go to war for it.

NicolasDorier · 2018-01-10T13:33:42Z

@andrasfuchs yes you can introduce bits and satoshi. I am quite 50/50% on using Bit or BTC in examples. Someone throw a coin! @andrasfuchs let us know when ready to merge.

Changed bits to BTC

andrasfuchs · 2018-01-10T20:26:59Z

@NicolasDorier @nopara73 I changed the text, please check it out. I'm leaving for 2 weeks tomorrow morning and I will have very limited internet access, but I will try my best if you need me to change anything else. Thank you for your patience!

andrasfuchs added 2 commits January 4, 2018 18:34

Changed wording according to issue ProgrammingBlockchain#93

042ef6e

Changed wording according to issue ProgrammingBlockchain#93

andrasfuchs mentioned this pull request Jan 4, 2018

About "transaction input or output" #93

Open

nopara73 previously requested changes Jan 4, 2018

View reviewed changes

NicolasDorier reviewed Jan 5, 2018

View reviewed changes

Changed bits to BTC

c4b37c7

Changed bits to BTC

nopara73 merged commit d2e020d into ProgrammingBlockchain:master Jan 10, 2018

Conversation

andrasfuchs commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

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!

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!

NicolasDorier Jan 5, 2018 • 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!

NicolasDorier commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrasfuchs commented Jan 5, 2018

Uh oh!

NicolasDorier commented Jan 9, 2018

Uh oh!

nopara73 commented Jan 9, 2018

Uh oh!

andrasfuchs commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nopara73 commented Jan 9, 2018

Uh oh!

NicolasDorier commented Jan 10, 2018

Uh oh!

andrasfuchs commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

andrasfuchs commented Jan 4, 2018 •

edited

Loading

NicolasDorier Jan 5, 2018 •

edited

Loading

NicolasDorier commented Jan 5, 2018 •

edited

Loading

andrasfuchs commented Jan 9, 2018 •

edited

Loading