Subcoin M2 Eval #1206

PieWol · 2024-08-12T12:49:40Z

No description provided.

liuchengxu · 2024-08-12T14:52:30Z

evaluations/subcoin_2_piewol.md

+
+## Documentation
+
+Some elements e.g. structs are still without inline doc. It would be great if this could be completed.


Could you point out which structs are missing docs? All the public items should be documented, I'm happy to fix them if not.

Just as an example:
https://subcoin-project.github.io/subcoin/sc_consensus_nakamoto/struct.ImportBlocks.html
https://subcoin-project.github.io/subcoin/pallet_bitcoin/fn.coin_storage_prefix.html

Also private items should be documented. It's not only relevant that people can use any code produced as a library, only understanding public api's. Instead we want to support software that is easy to maintain and understand in case somebody else picks it up for further development.

https://subcoin-project.github.io/subcoin/sc_consensus_nakamoto/struct.ImportBlocks.html
https://subcoin-project.github.io/subcoin/pallet_bitcoin/fn.coin_storage_prefix.html

Thanks for the links, docs updated and did another self-review on the code documentation.

Also private items should be documented. It's not only relevant that people can use any code produced as a library, only understanding public api's. Instead we want to support software that is easy to maintain and understand in case somebody else picks it up for further development.

Thanks for the feedback. While I appreciate the importance of documentation, I believe that code readability and maintainability are best achieved through clear and understandable code rather than relying heavily on inline documentation. Over-documenting can sometimes hurt the readability, making it harder to follow.

In my view, the primary focus should be on writing clean, self-explanatory code in the first place. If a piece of code is so complex that it requires extensive documentation to be understood, it might indicate an opportunity for refactoring. Simplifying the code can often make it more maintainable in the long run, reducing the need for extensive comments. Documentation should complement the code, not serve as a crutch to explain overly complicated logic. The balance lies in writing code that is intuitive and requires minimal explanation, supplemented by concise documentation where necessary.

I fully agree with you. Though, how self-explanatory some code really is, often is in the eye of the beholder. I just wanted to inform you about one of our reasons to require well documented code.

Thanks for checking the documentation and extending it.

liuchengxu · 2024-08-12T14:57:30Z

evaluations/subcoin_2_piewol.md

+
+In ``pallet-bitcoin`` you use the error message "txid must be encoded correctly; qed" both for encoding and decoding operations. I think it would be great it these error messages were unique for both operations. Furthermore it looks like the result type in the trait is used for a reason as the writer can emit errors. Whats the reason you put an `.expect()` here allowing for a panic? Basically, why are you sure that it will never happen? 
+
+Are you planning to introduce more tests to vital modules? ``pallet-bitcoin`` currently only has one test to check on encoding. Unit tests should cover the logic you have implemented.


The logic in pallet-bitcoin is super simple, its functionality is covered by the integration test, which is now run locally by hand and may be added to the CI pipeline in the future. The integration test case for pallet-bitcoin was introduced in the milestone 1, see https://github.com/subcoin-project/subcoin/blob/main/docs/src/usage.md#verify-the-utxo-set-state, for the objective of pallet-bitcoin is to maintain the UTXO state, as long as the UTXO state in pallet-bitcoin matches the Bitcoin state, it functions correctly.

got it, thanks for explaining.

liuchengxu · 2024-08-12T15:00:15Z

evaluations/subcoin_2_piewol.md

+# General Notes
+
+
+In ``pallet-bitcoin`` you use the error message "txid must be encoded correctly; qed" both for encoding and decoding operations. I think it would be great it these error messages were unique for both operations. Furthermore it looks like the result type in the trait is used for a reason as the writer can emit errors. Whats the reason you put an `.expect()` here allowing for a panic? Basically, why are you sure that it will never happen? 


Since pallet-bitcoin contains only one unsigned extrinsic, which is constructed internally, plus the Rust type bitcoin::Txid ensures the Bitcoin encode/decode works (guaranteed by rust-bitcoin), therefore the panic never happens.

liuchengxu · 2024-08-12T15:09:00Z

evaluations/subcoin_2_piewol.md

+| 0b.    | Documentation           | <ul><li>[ ] </li></ul> | Each module has its own docs. The rendered inline rustdoc is deployed at https://subcoin-project.github.io/subcoin/.  | some elements e.g. structs are still without inline doc |
+| 0c.    | Testing and Testing Guide | <ul><li>[x] </li></ul> |  https://github.com/subcoin-project/subcoin/tree/subcoin-milestone-2?tab=readme-ov-file#run-tests |  see general notes  |
+| 0d.    | Docker                  | <ul><li>[ ] </li></ul> | https://github.com/subcoin-project/subcoin/blob/subcoin-milestone-2/Dockerfile The docker image is available at https://github.com/subcoin-project/subcoin/pkgs/container/subcoin/249545041?tag=v0.2.0  |  builds. see notes. |
+| 1.     | Block Verification	       | <ul><li>[ ] </li></ul> | 	https://github.com/subcoin-project/subcoin/blob/subcoin-milestone-2/crates/sc-consensus-nakamoto/src/verification.rs |  how comes that there are no tests? Is this directly from ``rust-bitcoin``? |


Actually, there are two tests in the verification module :P, see https://github.com/subcoin-project/subcoin/blob/subcoin-milestone-2/crates/sc-consensus-nakamoto/src/verification/header_verify.rs#L266 and https://github.com/subcoin-project/subcoin/blob/subcoin-milestone-2/crates/sc-consensus-nakamoto/src/verification.rs#L569. I have to admit that the existing unit tests are not adequate as I haven't figured it out how to set up the entire test framework properly. The feature tests are now primarily conducted by running the node to sync the Bitcoin network locally. If the syncing is fine, it's probably fine (The existing two tests are spotted by the local syncing test). More comprehensive tests will be explored later.

Sounds good. Keen to see how more testing will be added later.

liuchengxu · 2024-08-12T15:11:49Z

evaluations/subcoin_2_piewol.md

+
+## Docker
+
+Sadly I can't figure out how to start the node in docker. If I try to use `docker run <image_id> run` I get the following error. Any idea how I can make it work?


I added a commit in the subcoin-milestone-2 branch that added the Docker command to run the node after submitting the delivery PR, can you try it again?

I will check it out and report back.

Nothing special with the docker build. It's probably something (maybe network issue) that occurred on your side since you have succeeded in building the image locally before. I'd suggest you try again and see if it works.

In addition, you can still use the image built by GitHub Action by following the command in the commit to run the Docker, for subcoin-project/subcoin@b35da7a contains no code changes to the subcoin-milestone-2 branch.

# For Docker users. docker run -d -v data:/node-data --user root --name subcoin ghcr.io/subcoin-project/subcoin:v0.2.0 run -d /node-data --log subcoin_network=debug # Check out the log. docker logs -f subcoin

liuchengxu · 2024-08-14T09:18:38Z

Ping @PieWol

PieWol · 2024-08-18T12:12:17Z

ping @liuchengxu ,
I will update my evaluation once I got docker to work. Thanks for your answers.

PieWol · 2024-08-18T12:40:08Z

I updated my evaluation, sadly the docker issue persists. Would be great if you could inform me about a solution.

liuchengxu · 2024-08-18T12:41:55Z

Can't reproduce the Docker issue on my end. Could you post the exact command you run on your machine? @PieWol

liuchengxu · 2024-08-18T12:53:29Z

evaluations/subcoin_2_piewol.md

+The dockerfile in the delivery release didn't seem to work. The node didn't start. With your specified hash `b35da7a` I still get the error, that the disk access is causing issues. 
+
+````
+root@ip-172-31-30-101:/home/ubuntu/subcoin# docker run subv2 run


If you have successfully built the image subv2 and failed to run with this exact command locally, please try adding --user root:

docker run --user root subv2 run

Using the root is not ideal, but it's not a high priority thing, let's just bear with it for now.

Cool, great thanks. Adding root worked. I was always under the impression that containers are always running as root. The more you know ^^.

github-actions · 2024-08-18T14:58:53Z

🪙 Please fill out the invoice form in order to initiate the payment process. Thank you!

PieWol added 3 commits August 12, 2024 10:14

init

8d479a9

eval update

8cf7726

docker not working

11f8074

PieWol mentioned this pull request Aug 12, 2024

Subcoin milestone 2 #1200

Merged

5 tasks

liuchengxu reviewed Aug 12, 2024

View reviewed changes

update eval

cb7c0a9

liuchengxu reviewed Aug 18, 2024

View reviewed changes

accept

dfaa8d7

PieWol merged commit 6324bb4 into w3f:master Aug 18, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subcoin M2 Eval #1206

Subcoin M2 Eval #1206

PieWol commented Aug 12, 2024

liuchengxu Aug 12, 2024

PieWol Aug 15, 2024

liuchengxu Aug 15, 2024

PieWol Aug 18, 2024 •

edited

Loading

liuchengxu Aug 12, 2024

PieWol Aug 18, 2024

liuchengxu Aug 12, 2024 •

edited

Loading

liuchengxu Aug 12, 2024

PieWol Aug 18, 2024

liuchengxu Aug 12, 2024

PieWol Aug 18, 2024 •

edited

Loading

liuchengxu Aug 18, 2024

liuchengxu commented Aug 14, 2024

PieWol commented Aug 18, 2024 •

edited

Loading

PieWol commented Aug 18, 2024

liuchengxu commented Aug 18, 2024

liuchengxu Aug 18, 2024

PieWol Aug 18, 2024

github-actions bot commented Aug 18, 2024


		## Documentation

		Some elements e.g. structs are still without inline doc. It would be great if this could be completed.


		In ``pallet-bitcoin`` you use the error message "txid must be encoded correctly; qed" both for encoding and decoding operations. I think it would be great it these error messages were unique for both operations. Furthermore it looks like the result type in the trait is used for a reason as the writer can emit errors. Whats the reason you put an `.expect()` here allowing for a panic? Basically, why are you sure that it will never happen?

		Are you planning to introduce more tests to vital modules? ``pallet-bitcoin`` currently only has one test to check on encoding. Unit tests should cover the logic you have implemented.

		# General Notes


		In ``pallet-bitcoin`` you use the error message "txid must be encoded correctly; qed" both for encoding and decoding operations. I think it would be great it these error messages were unique for both operations. Furthermore it looks like the result type in the trait is used for a reason as the writer can emit errors. Whats the reason you put an `.expect()` here allowing for a panic? Basically, why are you sure that it will never happen?


		## Docker

		Sadly I can't figure out how to start the node in docker. If I try to use `docker run <image_id> run` I get the following error. Any idea how I can make it work?

Subcoin M2 Eval #1206

Subcoin M2 Eval #1206

Conversation

PieWol commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PieWol Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuchengxu Aug 12, 2024 • 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

PieWol Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuchengxu commented Aug 14, 2024

PieWol commented Aug 18, 2024 • edited Loading

PieWol commented Aug 18, 2024

liuchengxu commented Aug 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2024

PieWol Aug 18, 2024 •

edited

Loading

liuchengxu Aug 12, 2024 •

edited

Loading

PieWol Aug 18, 2024 •

edited

Loading

PieWol commented Aug 18, 2024 •

edited

Loading