docs: fix various misleading comments

glozow commented at 2:04 pm on April 1, 2021: member

Came across a few misleading comments, wanted to fix them

fanquake added the label Docs on Apr 1, 2021

in src/chainparams.h:71 in 268965da59 outdated

68- * and services, the public test network which gets reset from time to time and
69+ * Bitcoin system. There are four: the main network on which people trade goods
70+ * and services, the public test network which gets reset from time to time,
71+ * a signet test network with extra block requirements (see BIP325) and
72  * a regression test mode which is intended for private networks only. It has
73  * minimal difficulty to ensure that blocks can be found instantly.

MarcoFalke commented at 2:12 pm on April 1, 2021:

nit: It might be better to move the explanation of each network to where each network is defined

jnewbery commented at 2:41 pm on April 1, 2021:

Agree. Better not to enumerate things here which can be documented where they’re defined.

laanwj commented at 3:04 pm on April 1, 2021:

Not sure here. I think there can be value in listing short descriptions in one place to have an overview. A more detailed description would belong with the individual chains, sure.

glozow commented at 3:48 pm on April 1, 2021:

Ah oops I didn’t see this comment! I suppose the issue with listing them here is that people might forget to update it, which is exactly what happened

in src/validation.cpp:1103 in 268965da59 outdated

1099@@ -1099,7 +1100,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp
1100     assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
1101     const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);
1102     if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
1103-        // Remove coins that were not present in the coins cache before calling ATMPW;
1104+        // Remove coins that were not present in the coins cache before MemPoolAccept;

jnewbery commented at 2:47 pm on April 1, 2021:

0        // Remove coins that were not present in the coins cache before calling.AcceptSingleTransaction();

?

jnewbery commented at 2:48 pm on April 1, 2021: member

ACK 268965da59bb35083faa4e697bfab3e9b53efbfd

Definitely improvements, but I’ve added one suggestion. I also agree with Marco about not documenting all of the chains in chainparams.h.

in src/chainparams.h:68 in 268965da59 outdated

63@@ -64,8 +64,9 @@ struct ChainTxData {
64 
65 /**
66  * CChainParams defines various tweakable parameters of a given instance of the
67- * Bitcoin system. There are three: the main network on which people trade goods
68- * and services, the public test network which gets reset from time to time and
69+ * Bitcoin system. There are four: the main network on which people trade goods
70+ * and services, the public test network which gets reset from time to time,

jonatack commented at 2:52 pm on April 1, 2021:

0 * and services, the public test network which is reset from time to time,

in src/validation.cpp:693 in 268965da59 outdated

689@@ -690,7 +690,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
690         }
691     }
692 
693-    // Bring the best block into scope
694+    // This is const, but calls into the backend CoinsViews. The CCoinsViewDB at the bottom of the

jonatack commented at 2:53 pm on April 1, 2021:

0    // This is const, but calls into the back end CoinsViews. The CCoinsViewDB at the bottom of the

jonatack commented at 2:55 pm on April 1, 2021: member

Concept ACK.

Agree with @MarcoFalke’s suggestion. Could squash into one commit. A couple of minor suggestions.

[doc] GetBestBlock() doesn't do nothing

This has tripped people up multiple times because it looks like
GetBestBlock is a const function returning the value of hashBlock.

2f8272c2a4

[doc] correct comment in chainparams

There are more than 3 networks.

8fa74aeb5b

[doc] correct comment about ATMPW

ATMPW stands for AcceptToMemoryPoolWorker, which was removed in #16400.

4eca20d6f7

glozow force-pushed on Apr 1, 2021

glozow commented at 3:37 pm on April 1, 2021: member

Thanks for the reviews, took everyone’s suggestions!

jnewbery commented at 4:06 pm on April 1, 2021: member

ACK 4eca20d6f7

MarcoFalke commented at 4:55 pm on April 1, 2021: member

ACK 4eca20d6f7d850492d331d89d1cdd77abb3c70c1

laanwj commented at 5:04 pm on April 1, 2021: member

Code review ACK 4eca20d6f7d850492d331d89d1cdd77abb3c70c1

laanwj merged this on Apr 1, 2021

laanwj closed this on Apr 1, 2021

sidhujag referenced this in commit ee65940ee0 on Apr 1, 2021

DrahtBot locked this on Aug 16, 2022

docs: fix various misleading comments #21567