💬 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.
💬 theStack commented on issue "Intermittent test failure in p2p_v2_transport":
(https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841751140)
I agree, but using `assert_debug_log` was not planned to be used for a potential solution anyway (note that parsing the log is not mentioned anywhere in my previous post). I would still use the `getpeerinfo()` RPC, but not look at the length, but wait for an increase of the highest peer id instead. At least that's the current plan, happy to take suggestions.
Regarding the interface, I think a context-manager method would make sense here, e.g.:
```
with self.nodes[0].wait_for_new_peer():
...
(https://github.com/bitcoin/bitcoin/issues/29002#issuecomment-1841751140)
I agree, but using `assert_debug_log` was not planned to be used for a potential solution anyway (note that parsing the log is not mentioned anywhere in my previous post). I would still use the `getpeerinfo()` RPC, but not look at the length, but wait for an increase of the highest peer id instead. At least that's the current plan, happy to take suggestions.
Regarding the interface, I think a context-manager method would make sense here, e.g.:
```
with self.nodes[0].wait_for_new_peer():
...
💬 theStack commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841774604)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841774604)
Concept ACK
💬 ryanofsky commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1416392404)
In commit "build: Prepare libbitcoin_util for internal kernel library" (8b3623769e1307c122959fbdc8c4c93ddd81d5c9)
It seems like a bug to have a util header depending on a common header. I think I'd suggest splitting up the spanparsing header and renaming it, since right now it is not very focused. Would suggest moving the spanparsing Split function to `src/util/string.h` where it is being used, and probably moving the rest of the functions to `src/script/parsing.h` since they are only used fo
...
(https://github.com/bitcoin/bitcoin/pull/28690#discussion_r1416392404)
In commit "build: Prepare libbitcoin_util for internal kernel library" (8b3623769e1307c122959fbdc8c4c93ddd81d5c9)
It seems like a bug to have a util header depending on a common header. I think I'd suggest splitting up the spanparsing header and renaming it, since right now it is not very focused. Would suggest moving the spanparsing Split function to `src/util/string.h` where it is being used, and probably moving the rest of the functions to `src/script/parsing.h` since they are only used fo
...
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416405430)
It was previously needed in a later commit when xpubs with private keys were being written, but it is no longer needed. I've ended up dropping this commit from this PR.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416405430)
It was previously needed in a later commit when xpubs with private keys were being written, but it is no longer needed. I've ended up dropping this commit from this PR.
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416405890)
Changed to `if(!Assume...`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416405890)
Changed to `if(!Assume...`
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416405961)
Done
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416405961)
Done
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416406078)
Done
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416406078)
Done
💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416406114)
Done
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1416406114)
Done
📝 theStack opened a pull request: "test: fix v2 transport intermittent test failure (#29002)"
(https://github.com/bitcoin/bitcoin/pull/29006)
This PR improves the following fragile construct for detection of a new connection to the node under test in `p2p_v2_transport.py`:
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/test/functional/p2p_v2_transport.py#L154-L156
Only relying on the number of peers for that suffers from race conditions, as unrelated previous peers could disconnect at anytime in-between. In the test run in #29002, the following happens:
- `getpeerinfo()` is called the first time
...
(https://github.com/bitcoin/bitcoin/pull/29006)
This PR improves the following fragile construct for detection of a new connection to the node under test in `p2p_v2_transport.py`:
https://github.com/bitcoin/bitcoin/blob/6d5790956f45e3de5c6c4ee6fda21878b0d1287b/test/functional/p2p_v2_transport.py#L154-L156
Only relying on the number of peers for that suffers from race conditions, as unrelated previous peers could disconnect at anytime in-between. In the test run in #29002, the following happens:
- `getpeerinfo()` is called the first time
...