refactor: Signet fixups

MarcoFalke commented at 1:59 pm on September 22, 2020: member

Some doc and test fixups for #18267

fanquake added the label Refactoring on Sep 22, 2020

in doc/bips.md:45 in faae52285a outdated

41@@ -42,4 +42,5 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.21.0**):
42 * [`BIP 173`](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki): Bech32 addresses for native Segregated Witness outputs are supported as of **v0.16.0** ([PR 11167](https://github.com/bitcoin/bitcoin/pull/11167)). Bech32 addresses are generated by default as of **v0.20.0** ([PR 16884](https://github.com/bitcoin/bitcoin/pull/16884)).
43 * [`BIP 174`](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki): RPCs to operate on Partially Signed Bitcoin Transactions (PSBT) are present as of **v0.17.0** ([PR 13557](https://github.com/bitcoin/bitcoin/pull/13557)).
44 * [`BIP 176`](https://github.com/bitcoin/bips/blob/master/bip-0176.mediawiki): Bits Denomination [QT only] is supported as of **v0.16.0** ([PR 12035](https://github.com/bitcoin/bitcoin/pull/12035)).
45+* [`BIP 325`](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki): Signet test network is supported as of **v0.21.0** ([PR 18267](https://github.com/bitcoin/bitcoin/pull/18267)).

ajtowns commented at 2:03 pm on September 22, 2020:

(Not strictly true until bitcoin/bips#978 and bitcoin/bips#1000 are merged)

MarcoFalke commented at 3:09 pm on September 22, 2020:

heh, I hope that those are merged before we release the next major version ;)

kallewoof commented at 3:09 pm on September 22, 2020:

I’ve ACK’d both, so they should be considered merged, I think, since i’m the BIP author.

in doc/release-notes.md:340 in faae52285a outdated

334@@ -335,6 +335,10 @@ RPC
335 Tests
336 -----
337 
338+- The BIP 325 default signet can be enabled by the `-chain=signet` or `-signet`
339+  setting. The settings `-signetchallenge` and `-signetseednode` allow
340+  modifying the network.

ajtowns commented at 2:04 pm on September 22, 2020:

“modifying the network” -> “enabling a custom signet instead” ?

MarcoFalke commented at 8:34 pm on September 22, 2020:

thx done

in src/signet.cpp:70 in fad3900969 outdated

64@@ -65,8 +65,9 @@ static uint256 ComputeModifiedMerkleRoot(const CMutableTransaction& cb, const CB
65     return ComputeMerkleRoot(std::move(leaves));
66 }
67 
68-SignetTxs SignetTxs::Create(const CBlock& block, const CScript& challenge)
69+Optional<SignetTxs> SignetTxs::Create(const CBlock& block, const CScript& challenge)
70 {
71+    const auto invalid = []() -> Optional<SignetTxs> { return nullopt; };

ajtowns commented at 2:38 pm on September 22, 2020:

Should just replace return invalid() with return nullopt directly.

MarcoFalke commented at 8:34 pm on September 22, 2020:

thx done

in src/signet.h:31 in fad3900969 outdated

34-    static SignetTxs Create(const CBlock& block, const CScript& challenge);
35+    SignetTxs(const T1& to_spend, const T2& to_sign) : m_to_spend{to_spend}, m_to_sign{to_sign} { }
36 
37 public:
38-    SignetTxs(const CBlock& block, const CScript& challenge) : SignetTxs(Create(block, challenge)) { }
39+    static Optional<SignetTxs> Create(const CBlock& block, const CScript& challenge);

ajtowns commented at 2:39 pm on September 22, 2020:

This is much better!

ajtowns commented at 2:41 pm on September 22, 2020: member

Concept ACK

DrahtBot commented at 6:59 pm on September 22, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
#11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fjahr commented at 8:18 pm on September 22, 2020: member

@ajtowns suggestions are good, ACK otherwise.

test: Run signet test even when wallet was not compiled fa2ad5dae1

refactor: Remove SignetTxs::m_valid and use optional instead

m_valid implies the block solution has been checked, which is not the
case. It only means the txs could be parsed. C++17 comes with
std::optional, so just use that instead.

77771a03df

fuzz: Remove needless guard fae0548686

doc: Update comments for new chain settings (-signet and -chain=signet) faf0a26711

doc: Document signet BIP facaf9e61f

MarcoFalke force-pushed on Sep 22, 2020

fjahr commented at 9:38 pm on September 22, 2020: member

Code review ACK fadee171c8b0b591daf8f1d65640e4f288b3c3bf

dr-orlovsky approved

dr-orlovsky commented at 10:53 pm on September 22, 2020: none

Reviewed & ACK https://github.com/bitcoin/bitcoin/pull/19993/commits/facaf9e61f4b9ea91fab554d495ebea1043d08fb

ajtowns commented at 1:33 am on September 23, 2020: member

ACK facaf9e61f4b9ea91fab554d495ebea1043d08fb – code review only

kallewoof commented at 2:17 am on September 23, 2020: member

Code review ACK facaf9e61f4b9ea91fab554d495ebea1043d08fb

MarcoFalke merged this on Sep 23, 2020

MarcoFalke closed this on Sep 23, 2020

MarcoFalke deleted the branch on Sep 23, 2020

sidhujag referenced this in commit edce812404 on Sep 23, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Signet fixups #19993

Conflicts