🤔 jonatack reviewed a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766097325)
ACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
This change would result in the following help:
```
$ ./src/bitcoin-cli -signet help encryptwallet
encryptwallet "passphrase"
Encrypts the wallet with 'passphrase'. This is for first time encryption.
After this, any calls that interact with private keys such as sending or signing
will require the passphrase to be set prior the making these calls.
Use the walletpassphrase call for this, and then walletlock call.
If the wallet is already
...
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766097325)
ACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
This change would result in the following help:
```
$ ./src/bitcoin-cli -signet help encryptwallet
encryptwallet "passphrase"
Encrypts the wallet with 'passphrase'. This is for first time encryption.
After this, any calls that interact with private keys such as sending or signing
will require the passphrase to be set prior the making these calls.
Use the walletpassphrase call for this, and then walletlock call.
If the wallet is already
...
💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841606774)
> Could alternatively use [-Wunreachable-code-aggressive](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-aggressive) -- it catches a few additional unreachable `break` statements in the current code.
They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841606774)
> Could alternatively use [-Wunreachable-code-aggressive](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-aggressive) -- it catches a few additional unreachable `break` statements in the current code.
They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841609784)
> But I'm not sure when this would happen except when the key can't be determined, which seems like an unusual case.
It would happen for all watch-only wallets, for example, although we do check for that flag separately.
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1841609784)
> But I'm not sure when this would happen except when the key can't be determined, which seems like an unusual case.
It would happen for all watch-only wallets, for example, although we do check for that flag separately.
💬 jonatack commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841613979)
> They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841613979)
> They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.
💬 achow101 commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841622415)
Perhaps a test for this case?
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841622415)
Perhaps a test for this case?
💬 jonatack commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416294937)
```suggestion
if table_name == "tried" and result["error"] == "failed-adding-to-tried":
```
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416294937)
```suggestion
if table_name == "tried" and result["error"] == "failed-adding-to-tried":
```
🤔 jonatack reviewed a pull request: "rpc: addpeeraddress tried return error on failure"
(https://github.com/bitcoin/bitcoin/pull/28998#pullrequestreview-1766137449)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28998#pullrequestreview-1766137449)
Concept ACK
💬 jonatack commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416295706)
Not sure, maybe test explicitly for `True` rather than just truthy
```suggestion
if result["success"] is True:
```
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1416295706)
Not sure, maybe test explicitly for `True` rather than just truthy
```suggestion
if result["success"] is True:
```
💬 techy2 commented on issue "getrawtransaction xxxxxx.... 2 causes a segfault":
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1841638292)
Over the years I have found a lot of obscure bugs in all kinds of code. I think karma pushes them towards me LOL
I'm 80+ and still finding them. I seem to have a knack for it... purely by accident.
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1841638292)
Over the years I have found a lot of obscure bugs in all kinds of code. I think karma pushes them towards me LOL
I'm 80+ and still finding them. I seem to have a knack for it... purely by accident.
💬 jonatack commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841640137)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841640137)
Concept ACK
💬 achow101 commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416301554)
Agree that this should be `NO_KEY_TIME`. Since wallet already includes spkm, leaving it there will not be circular, and it can still be accessed by things that include wallet.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416301554)
Agree that this should be `NO_KEY_TIME`. Since wallet already includes spkm, leaving it there will not be circular, and it can still be accessed by things that include wallet.
💬 mzumsande commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841662429)
I added a regression test (that will segfault on master). In order to trigger this, it's not enough to enable pruning, you have to actually have pruned a block, so I did this with `-fastprune` in the test. Because this will probably seem a bit random to someone reading the test without this context, I added a comment linking to this PR.
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841662429)
I added a regression test (that will segfault on master). In order to trigger this, it's not enough to enable pruning, you have to actually have pruned a block, so I did this with `-fastprune` in the test. Because this will probably seem a bit random to someone reading the test without this context, I added a comment linking to this PR.
💬 furszy commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1841668580)
Tackled both points. Thanks @jonatack.
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1841668580)
Tackled both points. Thanks @jonatack.
💬 theStack commented on issue "Intermittent test failure in p2p_v2_transport":
(https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841678647)
It seems that the problem is the following construct for detection of a new incoming connection:
```
num_peers = len(self.nodes[0].getpeerinfo())
s.connect(("127.0.0.1", p2p_port(0))
self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers + 1)
```
Looking at the logs, I think in this test run the the disconnect of the previous connection finished _after_ the first `getpeerinfo` RPC call, i.e. the peer count drops to `num_peers-1`, increases to `num_peers` after th
...
(https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841678647)
It seems that the problem is the following construct for detection of a new incoming connection:
```
num_peers = len(self.nodes[0].getpeerinfo())
s.connect(("127.0.0.1", p2p_port(0))
self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers + 1)
```
Looking at the logs, I think in this test run the the disconnect of the previous connection finished _after_ the first `getpeerinfo` RPC call, i.e. the peer count drops to `num_peers-1`, increases to `num_peers` after th
...
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416326180)
Done. Updated.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416326180)
Done. Updated.
💬 real-or-random commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1841695038)
Related: https://github.com/bitcoin-core/secp256k1/issues/1452
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1841695038)
Related: https://github.com/bitcoin-core/secp256k1/issues/1452
💬 maflcko commented on issue "Intermittent test failure in p2p_v2_transport":
(https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841702298)
It seems easier to add another `self.wait_until`.
Generally, I am not a fan of using `assert_debug_log`, because this may cause races such as this, and uses the debug log via polling to sync the test execution implicitly.
Adding an explicit wait on what should happen (disconnect) seems better than extracting a "magic" string from the debug log in a loop.
(https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841702298)
It seems easier to add another `self.wait_until`.
Generally, I am not a fan of using `assert_debug_log`, because this may cause races such as this, and uses the debug log via polling to sync the test execution implicitly.
Adding an explicit wait on what should happen (disconnect) seems better than extracting a "magic" string from the debug log in a loop.
🤔 pablomartin4btc reviewed a pull request: "build: Introduce internal kernel library"
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1766232622)
Concept ACK
I'd recommend reading Review Club [notes](https://bitcoincore.reviews/28690) from @stickies-v (thanks!) since those are great, well structured and very useful in order to have a wider context of most of the changes involved.
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1766232622)
Concept ACK
I'd recommend reading Review Club [notes](https://bitcoincore.reviews/28690) from @stickies-v (thanks!) since those are great, well structured and very useful in order to have a wider context of most of the changes involved.
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-1841742823)
Rebased 29818f5f839bb0a08980da6f6dbb980ef7754652 -> 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf ([mempoolArgs_0](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_0) -> [mempoolArgs_1](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_0..mempoolArgs_1))
Updated 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf -> 3aa41a31690da2ae22b6a15a9a5de0de78a01757 ([mempoolArgs_1](https://github.com/TheCharlatan/bitcoin/tree/me
...
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-1841742823)
Rebased 29818f5f839bb0a08980da6f6dbb980ef7754652 -> 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf ([mempoolArgs_0](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_0) -> [mempoolArgs_1](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_0..mempoolArgs_1))
Updated 1d18cc6cb30a875c4b6bea946a8c19d42e76dbcf -> 3aa41a31690da2ae22b6a15a9a5de0de78a01757 ([mempoolArgs_1](https://github.com/TheCharlatan/bitcoin/tree/me
...
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1416362699)
Uff, embarassing.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1416362699)
Uff, embarassing.