Remove -deprecatedrpc=addresses flag and corresponding code/logic #22650

mjdietzx commented at 2:29 PM on August 6, 2021: contributor

Resolves #21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.

-deprecatedrpc=addresses was initially added in this PR #20286 (which resolved the original issue #20102).

Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)

mjdietzx force-pushed on Aug 6, 2021

DrahtBot added the label RPC/REST/ZMQ on Aug 6, 2021

DrahtBot added the label Utils/log/libs on Aug 6, 2021

DrahtBot added the label Wallet on Aug 6, 2021

DrahtBot commented at 5:09 AM on August 7, 2021: 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:

#23091 (fuzz: fix checks on number of required sigs and keys in multisig scripts by mjdietzx)
#22951 (consensus: move amount.h into consensus by fanquake)
#22918 (rpc: Add level 3 verbosity to getblock RPC call (#21245 modified) by kiminuo)
#20295 (rpc: getblockfrompeer by Sjors)
#16795 (rpc: have raw transaction decoding infer output descriptors by instagibbs)

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.

fanquake added this to the milestone 23.0 on Aug 7, 2021

fanquake commented at 2:24 AM on August 12, 2021: member

Concept ACK

theStack commented at 8:35 AM on August 13, 2021: member

Concept ACK

MarcoFalke commented at 8:51 AM on August 25, 2021: member

review ACK 28bd30a73bd4bff1dc44c89ae64a194cc1c526e4 📰

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash f308d8d97552a7b726d0344e7ac0ace93281130a51a3154b19a1b6b4e0c1e1d2 -

</details>

MarcoFalke commented at 8:55 AM on August 25, 2021: member

Oh, in the release notes it should say v22 and bitcoin-tx had no behaviour change at all, so no release notes needed?!

mjdietzx force-pushed on Aug 25, 2021

mjdietzx commented at 1:07 PM on August 25, 2021: contributor

Thanks @MarcoFalke, release notes have been corrected

in src/core_write.cpp:152 in 3add69bab3 outdated

 175 | -    if (!ExtractDestinations(scriptPubKey, type, addresses, nRequired) || type == TxoutType::PUBKEY) {
 176 | -        out.pushKV("type", GetTxnOutputType(type));
 177 | -        return;
 178 | -    }
 179 | +    std::vector<std::vector<unsigned char>> solns;
 180 | +    TxoutType type = Solver(scriptPubKey, solns);

jonatack commented at 1:58 PM on August 25, 2021:

b0770cee942e17f6ed3d4

    const TxoutType type{Solver(scriptPubKey, solns)};

mjdietzx commented at 8:02 PM on August 25, 2021:

Done a1c97440fd8d02c8e931597f6dac2237346b8330

in src/core_write.cpp:224 in 3add69bab3 outdated

 215 | @@ -249,7 +216,7 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, bool include_add
 216 |          out.pushKV("n", (int64_t)i);
 217 |  
 218 |          UniValue o(UniValue::VOBJ);
 219 | -        ScriptPubKeyToUniv(txout.scriptPubKey, o, true, include_addresses);
 220 | +        ScriptPubKeyToUniv(txout.scriptPubKey, o, true);

jonatack commented at 1:59 PM on August 25, 2021:

Named args could be nice wherever this function is invoked

        ScriptPubKeyToUniv(txout.scriptPubKey, /* out */ o, /* include_hex */ true);

mjdietzx commented at 8:02 PM on August 25, 2021:

Done 0ba1fe416a020395877a09f19f86df5e088b4624

in src/test/fuzz/script.cpp:112 in 3add69bab3 outdated

 108 | @@ -133,15 +109,11 @@ FUZZ_TARGET_INIT(script, initialize_script)
 109 |      (void)ScriptToAsmStr(script, true);
 110 |  
 111 |      UniValue o1(UniValue::VOBJ);
 112 | -    ScriptPubKeyToUniv(script, o1, true, true);
 113 | -    ScriptPubKeyToUniv(script, o1, true, false);
 114 | +    ScriptPubKeyToUniv(script, o1, true);

jonatack commented at 2:08 PM on August 25, 2021:

b0770cee942e17f6ed3d4ded wherever this function is called

    ScriptPubKeyToUniv(script, /* out */ o1, /* include_hex */ true);

in src/wallet/rpcwallet.cpp:1765 in 3add69bab3 outdated

1753 | @@ -1754,7 +1754,7 @@ static RPCHelpMan gettransaction()
1754 |  
1755 |      if (verbose) {
1756 |          UniValue decoded(UniValue::VOBJ);
1757 | -        TxToUniv(*wtx.tx, uint256(), pwallet->chain().rpcEnableDeprecated("addresses"), decoded, false);
1758 | +        TxToUniv(*wtx.tx, uint256(), decoded, false);

jonatack commented at 2:11 PM on August 25, 2021:

b0770cee942e17f6ed3d4ded

        TxToUniv(*wtx.tx, /* hashBlock */ uint256(), /* entry */ decoded, /* include_hex */ false);

in src/core_io.h:48 in 3add69bab3 outdated

  43 | @@ -44,8 +44,8 @@ UniValue ValueFromAmount(const CAmount amount);
  44 |  std::string FormatScript(const CScript& script);
  45 |  std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0);
  46 |  std::string SighashToStr(unsigned char sighash_type);
  47 | -void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool fIncludeHex, bool include_addresses);
  48 | -void ScriptToUniv(const CScript& script, UniValue& out, bool include_address);
  49 | -void TxToUniv(const CTransaction& tx, const uint256& hashBlock, bool include_addresses, UniValue& entry, bool include_hex = true, int serialize_flags = 0, const CTxUndo* txundo = nullptr);
  50 | +void ScriptPubKeyToUniv(const CScript& scriptPubKey, UniValue& out, bool include_hex, bool include_address = true);
  51 | +inline void ScriptToUniv(const CScript& script, UniValue& out) { ScriptPubKeyToUniv(script, out, /* include_hex */ true, /* include_address */ false); }

jonatack commented at 2:29 PM on August 25, 2021:

6604cdcccd maybe I'm missing something but perhaps merge ScriptPubKeyToUniv and ScriptToUniv into a single function?

mjdietzx commented at 8:04 PM on August 25, 2021:

I thought about it (and agree it's a good change). However I didn't include it originally because I wanted to be conservative w/ the changes in this PR and thought keeping them as separate functions may have been "self documenting"

Anyways, I wanted to do this and since you were thinking the same thing I did it in this commit b456004de9ef61716140c9020c572416b46acbff - if we decide we don't want it I can just revert this commit

MarcoFalke commented at 2:32 PM on August 25, 2021: member

re-ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2 🚅

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash e9ce8225e5bc9dceb1997a8dd371a81415ce8afa17aee27a36cf95c220ae6d1f -

</details>

in doc/release-notes.md:71 in 3add69bab3 outdated

  63 | @@ -64,6 +64,12 @@ P2P and network changes
  64 |  Updated RPCs
  65 |  ------------
  66 |  
  67 | +- The `-deprecatedrpc=addresses` flag has been removed entirely.
  68 | +  The following RPCs:  `gettxout`, `getrawtransaction`, `decoderawtransaction`,
  69 | +  `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`,
  70 | +  `/rest/block` no longer return the fields: `addresses`, `reqSigs` which were
  71 | +  previously deprecated in v22. (#22650)

jonatack commented at 2:39 PM on August 25, 2021:

IIUC gettransaction is only affected when verbose=true and some writing suggestions

-- The `-deprecatedrpc=addresses` flag has been removed entirely.
-  The following RPCs:  `gettxout`, `getrawtransaction`, `decoderawtransaction`,
-  `decodescript`, `gettransaction`, and REST endpoints: `/rest/tx`, `/rest/getutxos`,
-  `/rest/block` no longer return the fields: `addresses`, `reqSigs` which were
-  previously deprecated in v22. (#22650)
+- The `-deprecatedrpc=addresses` configuration option has been removed.  RPCs
+  `gettxout`, `getrawtransaction`, `decoderawtransaction`, `decodescript`,
+  `gettransaction verbose=true` and REST endpoints `/rest/tx`, `/rest/getutxos`,
+  `/rest/block` no longer return the `addresses` and `reqSigs` fields, which
+  were previously deprecated in 22.0. (#22650)

mjdietzx commented at 8:04 PM on August 25, 2021:

Done: 8ede37671d1174793841ee70e4f21e3221b5b82a

jonatack commented at 2:49 PM on August 25, 2021: member

ACK 3add69bab36ce8e3d1d7d0664d264e4c163233c2

Happy to re-ACK if you update.

mjdietzx force-pushed on Aug 25, 2021

mjdietzx commented at 8:06 PM on August 25, 2021: contributor

Thanks for the review @jonatack - I made all your suggestions. I had to edit some commits and force push. I'm hoping this didn't throw a wrench in your review @MarcoFalke . If it did and you know a better way I could've done it please lmk so I can avoid this in the future 😬

in src/rpc/rawtransaction.cpp:1039 in b456004de9 outdated

1035 | @@ -1056,7 +1036,7 @@ static RPCHelpMan decodepsbt()
1036 |                                          {RPCResult::Type::STR, "asm", "The asm"},
1037 |                                          {RPCResult::Type::STR_HEX, "hex", "The hex"},
1038 |                                          {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
1039 | -                                        {RPCResult::Type::STR, "address"," Bitcoin address if there is one"},
1040 | +                                        {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (only if a well-defined address exists)"},

jonatack commented at 10:27 AM on September 2, 2021:

b0770cee ignore unless you retouch

                                        {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},

("Bitcoin" seems to be a proper noun here, look at the translations returned by git grep -ni "bitcoin address")

mjdietzx commented at 2:27 AM on September 3, 2021:

I went ahead and made this improvement in all places for this field, which was added in the first PR #20286

in src/rpc/rawtransaction.cpp:1172 in b456004de9 outdated

1168 | @@ -1189,7 +1169,7 @@ static RPCHelpMan decodepsbt()
1169 |              txout = input.non_witness_utxo->vout[psbtx.tx->vin[i].prevout.n];
1170 |  
1171 |              UniValue non_wit(UniValue::VOBJ);
1172 | -            TxToUniv(*input.non_witness_utxo, uint256(), non_wit, false);
1173 | +            TxToUniv(*input.non_witness_utxo, uint256(), /* entry */ non_wit, /* include_hex */ false);

jonatack commented at 10:47 AM on September 2, 2021:

0ba1fe4 (only if you retouch) missed one ;)

            TxToUniv(*input.non_witness_utxo, /* hashBlock */ uint256(), /* entry */ non_wit, /* include_hex */ false);

mjdietzx commented at 2:27 AM on September 3, 2021:

Nice catch, done

jonatack commented at 11:22 AM on September 2, 2021: member

Great cleanup. Rebased to current master and debug-built each commit. It may be good to drop 7f7a2d9 "Inline ScriptToUniv", i.e. merge the two commits, since ScriptToUniv() is un-inlined right after again in b456004. ACK b456004de9ef61716140c9020c572416b46acbff otherwise.

mjdietzx force-pushed on Sep 3, 2021

mjdietzx commented at 2:30 AM on September 3, 2021: contributor

Thanks for the re review @jonatack !

It may be good to drop 7f7a2d9 "Inline ScriptToUniv", i.e. merge the two commits, since ScriptToUniv() is un-inlined right after again in b456004

I squashed these two commits together. And then addressed both of your comments

jonatack commented at 1:12 PM on September 3, 2021: member

Code review re-ACK 92dc5e954fdb974160198f0c97eff3e7e51c1372 only changes are documentation and squashing two commits to one, checked git diff b456004 92dc5e9, re-read the per-commit changes, and rebased to current master/debug build/checked unit+functional tests pass

MarcoFalke commented at 11:14 AM on September 7, 2021: member

NACK 92dc5e954f

No idea what this commit is trying to achieve. Maybe split this up into another pull? It is dangerous by creating undetectable silent merge conflicts and not to mention it is plain wrong.

review ACK 073035f5a6fae435b96426273f6d1930fc6a70e5 🛎

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 70af3366d8fa8d42efbe696d981448bff0730a1f79b3417951cb6e66fe628fa9 -

</details>

in src/rpc/rawtransaction.cpp:1270 in 92dc5e954f outdated

1267 |              out.pushKV("redeem_script", r);
1268 |          }
1269 |          if (!output.witness_script.empty()) {
1270 |              UniValue r(UniValue::VOBJ);
1271 | -            ScriptToUniv(output.witness_script, /* out */ r);
1272 | +            ScriptToUniv(output.witness_script, /* out */ r, /* include_address */ false);

MarcoFalke commented at 11:15 AM on September 7, 2021:

the named arg is wrong

MarcoFalke changes_requested

mjdietzx force-pushed on Sep 8, 2021

mjdietzx commented at 2:51 AM on September 8, 2021: contributor

@MarcoFalke, I think you're right about not including 92dc5e954fdb974160198f0c97eff3e7e51c1372 in this PR. I removed it and will open a 2nd PR with only that commit so it can be reviewed on its own merits.

DrahtBot added the label Needs rebase on Sep 9, 2021

in src/test/fuzz/transaction.cpp:106 in ba551e1b52 outdated

 102 | @@ -103,6 +103,6 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
 103 |      (void)IsWitnessStandard(tx, coins_view_cache);
 104 |  
 105 |      UniValue u(UniValue::VOBJ);
 106 | -    TxToUniv(tx, /* hashBlock */ uint256::ZERO, u);
 107 | -    TxToUniv(tx, /* hashBlock */ uint256::ONE, u);
 108 | +    TxToUniv(tx, /* hashBlock */ uint256::ZERO, /* entry */ u);

MarcoFalke commented at 1:22 PM on September 9, 2021:

I think you can drop one of these statements to make the fuzz test run ~twice as fast.

(In a separate commit, obviously)

mjdietzx commented at 10:40 PM on September 11, 2021:

Good point. I don't see any need to exercise this code path from the fuzz test, which seems to be the only point of invoking TxToUniv again w/ /* hashBlock */ uint256::ONE (especially if it will run twice as fast)

if (!hashBlock.IsNull())
  entry.pushKV("blockhash", hashBlock.GetHex());

So I'll add a commit removing TxToUniv(tx, /* hashBlock */ uint256::ONE, u); as suggested.

mjdietzx commented at 2:19 PM on September 14, 2021:

Done in this commit c45b13389a3540a07c0f64f63cb36f538f8e2f1c

MarcoFalke commented at 2:29 PM on September 14, 2021:

An alternative (to test both code paths) would be to deserialize a bool from the fuzzing input and use uint256{bool_val}

MarcoFalke commented at 2:31 PM on September 14, 2021:

Can also be done in a follow-up

in src/wallet/rpcwallet.cpp:1762 in ba551e1b52 outdated

1753 | @@ -1754,7 +1754,7 @@ static RPCHelpMan gettransaction()
1754 |  
1755 |      if (verbose) {
1756 |          UniValue decoded(UniValue::VOBJ);
1757 | -        TxToUniv(*wtx.tx, uint256(), decoded, false);
1758 | +        TxToUniv(*wtx.tx, /* hashBlock */ uint256(), /* entry */ decoded, /* include_hex */ false);

MarcoFalke commented at 1:24 PM on September 9, 2021:

If you keep this commit in this pull, it could make sense to use the /* arg= */ syntax, so that clang-tidy understands it as well.

mjdietzx commented at 2:29 PM on September 14, 2021:

Good suggestion. My plan is to do this and squash it into a0f2dab816fc9987fb177c02f5d801cf0b8ebbf9

in src/rpc/rawtransaction.cpp:494 in ba551e1b52 outdated

 494 | -                                    {RPCResult::Type::STR, "address", /* optional */ true, "bitcoin address (only if a well-defined address exists)"},
 495 | -                                    {RPCResult::Type::ARR, "addresses", /* optional */ true, "(DEPRECATED, returned only if config option -deprecatedrpc=addresses is passed) Array of bitcoin addresses",
 496 | -                                    {
 497 | -                                        {RPCResult::Type::STR, "address", "bitcoin address"},
 498 | -                                    }},
 499 | +                                    {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},

MarcoFalke commented at 1:26 PM on September 9, 2021:

as a follow-up it could make sense to de-duplicate the common help text

mjdietzx commented at 2:31 PM on September 14, 2021:

Agreed this could be a worthwhile cleanup, there are 6 occurrences of this exact string, 5 of which are in the same file src/rpc/rawtransaction.cpp

Do you have any suggestion on where this variable belongs or can you point me to any conventions in the code?

MarcoFalke commented at 2:33 PM on September 14, 2021:

See:

 static const std::vector<RPCResult> TransactionDescriptionString()

mjdietzx force-pushed on Sep 14, 2021

mjdietzx commented at 2:21 PM on September 14, 2021: contributor

Rebased.

I think you can drop one of these statements to make the fuzz test run ~twice as fast.

Done c45b133

in src/bitcoin-tx.cpp:730 in a0f2dab816 outdated

 726 | @@ -727,7 +727,7 @@ static void MutateTx(CMutableTransaction& tx, const std::string& command,
 727 |  static void OutputTxJSON(const CTransaction& tx)
 728 |  {
 729 |      UniValue entry(UniValue::VOBJ);
 730 | -    TxToUniv(tx, uint256(), entry);
 731 | +    TxToUniv(tx, /* hashBlock */ uint256(), entry);

MarcoFalke commented at 2:28 PM on September 14, 2021:

a0f2dab816fc9987fb177c02f5d801cf0b8ebbf9: I think this can be done in a follow-up.

If you decide to do it here, at least use the proper clang-tidy format, so that it can be checked by the clang compiler.

https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-argument-comment.html

, /* hashBlock=*/ uint256{},

mjdietzx commented at 8:21 PM on September 14, 2021:

I am moving the commit this relates to into a follow-up, and will do as you suggest there. Resolving this for now

MarcoFalke commented at 2:30 PM on September 14, 2021: member

review ACK f1acb5434f 🐯

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 1eb7fc34e86b60e73cdd3193dc2eeb609ebf87c032e3483f7067eb23fe71b73a -

</details>

DrahtBot removed the label Needs rebase on Sep 14, 2021

mjdietzx force-pushed on Sep 14, 2021

mjdietzx commented at 8:25 PM on September 14, 2021: contributor

I got rid of two commits I thought were slightly unnecessary and would be better off in a followup refactor PR: c45b13389a3540a07c0f64f63cb36f538f8e2f1c fuzz: remove an unuseful TxToUniv invocation to speedup fuzz/transaction.cpp a0f2dab816fc9987fb177c02f5d801cf0b8ebbf9 refactor: named args whenever ScriptToUniv, ScriptPubKeyToUniv, TxToUniv are invoked

I will be making the changes @MarcoFalke suggested in the followup PR as the only remaining requested changes from this review were related to those last commits.

MarcoFalke approved

MarcoFalke commented at 7:25 AM on September 15, 2021: member

my previous review is now valid

DrahtBot added the label Needs rebase on Sep 23, 2021

in src/test/fuzz/script.cpp:76 in f1acb5434f outdated

  71 | -                   type_ret == TxoutType::NONSTANDARD ||
  72 | -                   type_ret == TxoutType::NULL_DATA);
  73 | -        }
  74 | -    } else {
  75 | -        assert(required_ret >= 1 && required_ret <= 16);
  76 | -        assert((unsigned long)required_ret == addresses.size());

MarcoFalke commented at 4:09 PM on September 24, 2021:

This assertion fails:

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39152

It might be good to merge this pull request or apply the changes to this fuzz test early.

mjdietzx commented at 7:24 PM on September 24, 2021:

I'm looking into this now to see why this assertion failed. These tests were added in #20772. I'm also curious why we're only seeing this now but haven't had the chance to look into this and know what's going on

mjdietzx commented at 12:11 AM on September 25, 2021:

I just went ahead and fixed that assertion as I believe it was a buggy assertion. I didn't want this PR to get rushed bc of this, and figured we may want this for correctness. #23091 -- I realize the PR is kinda pointless, so if its best to close it and this is ready to merge, just lmk

rpc: remove deprecated addresses and reqSigs from rpc outputs 8721638daa

refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv d64deac7b8

refactor: minor styling, prefer snake case and same line if 2b1fdc2c6c

doc: add release notes for removal of the -deprecatedrpc=addresses flag 43cd6b8af9

mjdietzx force-pushed on Sep 24, 2021

mjdietzx commented at 7:23 PM on September 24, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Sep 24, 2021

mjdietzx requested review from jonatack on Sep 24, 2021

MarcoFalke commented at 7:38 AM on September 25, 2021: member

re-ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec 🐉

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 1ef8757c6c687d7a663bef23283581d4e088670176d49a29213e78ffea20beec -

</details>

in src/test/fuzz/script.cpp:116 in d64deac7b8 outdated

 112 | @@ -113,9 +113,7 @@ FUZZ_TARGET_INIT(script, initialize_script)
 113 |      UniValue o2(UniValue::VOBJ);
 114 |      ScriptPubKeyToUniv(script, o2, false);
 115 |      UniValue o3(UniValue::VOBJ);
 116 | -    ScriptToUniv(script, o3, true);
 117 | -    UniValue o4(UniValue::VOBJ);
 118 | -    ScriptToUniv(script, o4, false);
 119 | +    ScriptToUniv(script, o3);

meshcollider commented at 1:07 AM on September 28, 2021:

This should really be replaced with include_address=true/false checks on ScriptPubKeyToUniv(script, o4, fIncludeHex, include_address)

mjdietzx commented at 1:19 AM on September 28, 2021:

Good point, this isn't really needed. But because the followup PR #22924 merges ScriptPubKeyToUniv and ScriptToUniv into a single function it will be getting rid of this anyways. So this will be removed in the followup

meshcollider commented at 1:11 AM on September 28, 2021: contributor

Code review ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec

Commit refactor: minor styling, prefer snake case and same line if should be squashed with the previous refactoring commit.

in src/core_write.cpp:154 in 43cd6b8af9

 170 | -    int nRequired;
 171 |  
 172 |      out.pushKV("asm", ScriptToAsmStr(scriptPubKey));
 173 | -    if (fIncludeHex)
 174 | -        out.pushKV("hex", HexStr(scriptPubKey));
 175 | +    if (include_hex) out.pushKV("hex", HexStr(scriptPubKey));

jonatack commented at 1:28 PM on September 28, 2021:

Style thought only (ignore here), in this case where the conditional is for an action other than, say, a simple guard clause with return or return true/false, I increasingly add braces instead of one-lining it. The action stands out, and if a line raises while running gdb, it's more isolated. Others may have a different opinion, so just a thought.

mjdietzx commented at 1:26 AM on September 29, 2021:

Done in followup https://github.com/bitcoin/bitcoin/pull/22924/files#diff-48fa743e653b0ceebb1e454766c5a2e70981a44503eca6adf24895016db9eb26R149

jonatack commented at 1:32 PM on September 28, 2021: member

ACK 43cd6b8af9d613ca033800c5cd8524c3f77e13ec per git range-diff a9d0cec 92dc5e9 43cd6b8, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit

meshcollider merged this on Sep 28, 2021

meshcollider closed this on Sep 28, 2021

DrahtBot locked this on Oct 30, 2022