💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805112369)
b96beb27d2b3d06a45644c537c114a08f0ccd285
Love this, great optimization and makes a lot of sense.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805112369)
b96beb27d2b3d06a45644c537c114a08f0ccd285
Love this, great optimization and makes a lot of sense.
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1803679987)
1bfc1ca9b6b68c6356ffc23ecf01e417152ade95
Why do CNode .addr and .addrBind still need to be CAddress? maybe it makes some sense for .addr if that's where we also store the service flags for easier gossiping, but why would the local bind address need anything besides an IP and port?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1803679987)
1bfc1ca9b6b68c6356ffc23ecf01e417152ade95
Why do CNode .addr and .addrBind still need to be CAddress? maybe it makes some sense for .addr if that's where we also store the service flags for easier gossiping, but why would the local bind address need anything besides an IP and port?
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805115729)
b96beb27d2b3d06a45644c537c114a08f0ccd285
Why not use `GetNewNodeId()` here? (and below) Don't you have a Connman with its own `m_next_node_id`?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805115729)
b96beb27d2b3d06a45644c537c114a08f0ccd285
Why not use `GetNewNodeId()` here? (and below) Don't you have a Connman with its own `m_next_node_id`?
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805134391)
50cb52470ea6cc4de5a5e5260b8f308353942ec0
This is great, can be used for timed actions (like locking the wallet) we used to depend on libevent for
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805134391)
50cb52470ea6cc4de5a5e5260b8f308353942ec0
This is great, can be used for timed actions (like locking the wallet) we used to depend on libevent for
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805157675)
3bb0145514978092ae966e30693cf619e4034837
the `else {error}` looked backwards to me here at first, might be more clear to use `== 0` instead of `!` but i dunno what the official style is for this.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805157675)
3bb0145514978092ae966e30693cf619e4034837
the `else {error}` looked backwards to me here at first, might be more clear to use `== 0` instead of `!` but i dunno what the official style is for this.
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805124507)
bb5b91d430026c4826a2107c8c1a311518cc97ce
nit s/temporary/temporarily
and above
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805124507)
bb5b91d430026c4826a2107c8c1a311518cc97ce
nit s/temporary/temporarily
and above
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1804970448)
b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc
Reminding myself to check for a test that covers a child class with i2p true but no Event listener
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1804970448)
b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc
Reminding myself to check for a test that covers a child class with i2p true but no Event listener
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1804998403)
b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc
Seems weird to me to move the unique pointer to a connected socket out of sockman, implying that sockman itself doesn't own the connections. Peeking ahead at future commits though I think this gets resolved.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1804998403)
b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc
Seems weird to me to move the unique pointer to a connected socket out of sockman, implying that sockman itself doesn't own the connections. Peeking ahead at future commits though I think this gets resolved.
📝 sipa opened a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112)
Builds on top of #31097. Fixes #30960.
So far, detailed information about script validation failures is only reported when running with `-par=1`, due to a lack of ability to transfer information from the script validation threads to the validation thread. Fix this by extending the `CCheckQueue` functionality to pass more results through than just success/failure, and use this to report the exact Script error, as well as the transaction input in which it occurred.
(https://github.com/bitcoin/bitcoin/pull/31112)
Builds on top of #31097. Fixes #30960.
So far, detailed information about script validation failures is only reported when running with `-par=1`, due to a lack of ability to transfer information from the script validation threads to the validation thread. Fix this by extending the `CCheckQueue` functionality to pass more results through than just success/failure, and use this to report the exact Script error, as well as the transaction input in which it occurred.
🤔 MarnixCroes reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2378402702)
cACK good clarification
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2378402702)
cACK good clarification
💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806678697)
```suggestion
"\nLoad wallet from the wallet dir:\n"
```
this is easier to understand imo, and is correct for default cases and when -walletdir is specified
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806678697)
```suggestion
"\nLoad wallet from the wallet dir:\n"
```
this is easier to understand imo, and is correct for default cases and when -walletdir is specified
💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806705701)
```suggestion
+ HelpExampleCli("loadwallet", "\"/path/to/walletname/\"")
```
more clear imo
same for other ones
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806705701)
```suggestion
+ HelpExampleCli("loadwallet", "\"/path/to/walletname/\"")
```
more clear imo
same for other ones
💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806697883)
in what case would a user have this?
seems non standard, and not worth having an example for imo.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806697883)
in what case would a user have this?
seems non standard, and not worth having an example for imo.
💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806675500)
```suggestion
+ HelpExampleCli("loadwallet", "\"walletname\"")
```
_walletname_ is easier to understand than _foo_ imo
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806675500)
```suggestion
+ HelpExampleCli("loadwallet", "\"walletname\"")
```
_walletname_ is easier to understand than _foo_ imo
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806727152)
I think this should be `const CTransactionRef& tx` (and in the header file as well)
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806727152)
I think this should be `const CTransactionRef& tx` (and in the header file as well)
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806730678)
Perhaps we could let `bypass_limits` be filled in by the fuzzer, rather than hardcoded to false?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806730678)
Perhaps we could let `bypass_limits` be filled in by the fuzzer, rather than hardcoded to false?
💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806765767)
Overall I think this is much easier to follow with the codepaths merged, thanks. However on further review, I am concerned about how much work we might do in this function.
As written, if a transaction had 1000 inputs, which all spend from a single parent with 1000 outputs, then we'd do 1M loop iterations, because we're not deduplicating parents for each input of a transaction (so for each input of the child, we'd be looking at all 1000 outputs of the parent). If we deduplicate the parents
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806765767)
Overall I think this is much easier to follow with the codepaths merged, thanks. However on further review, I am concerned about how much work we might do in this function.
As written, if a transaction had 1000 inputs, which all spend from a single parent with 1000 outputs, then we'd do 1M loop iterations, because we're not deduplicating parents for each input of a transaction (so for each input of the child, we'd be looking at all 1000 outputs of the parent). If we deduplicate the parents
...
⚠️ casey opened an issue: "COIN_VALUE no longer accessible in const contexts"
(https://github.com/bitcoin/bitcoin/issues/31113)
The `COIN_VALUE` const was removed, I assume in favor of `Amount::ONE_BTC`, however, if you need a `u64` in a const context, I don't think you can get it because `Amount::to_sat` is not marked `const`. I think this could be allowed by making `Amount::to_sat` const.
(https://github.com/bitcoin/bitcoin/issues/31113)
The `COIN_VALUE` const was removed, I assume in favor of `Amount::ONE_BTC`, however, if you need a `u64` in a const context, I don't think you can get it because `Amount::to_sat` is not marked `const`. I think this could be allowed by making `Amount::to_sat` const.
🤔 Christewart reviewed a pull request: "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped"
(https://github.com/bitcoin/bitcoin/pull/31040#pullrequestreview-2378904148)
I'm a bit confused to the behavior i'm observing on this PR.
I tried adding another part to the test case to check we can clear the orphanage by propogating parent transactions: https://github.com/Christewart/bitcoin/commit/04815c463d331f674f67fdcb3b8da601484d7a33
It appears that to _totally_ clear the orphanage, we need to propagate the parent transaction for the last child transaction, which is unexpected I believe?
(https://github.com/bitcoin/bitcoin/pull/31040#pullrequestreview-2378904148)
I'm a bit confused to the behavior i'm observing on this PR.
I tried adding another part to the test case to check we can clear the orphanage by propogating parent transactions: https://github.com/Christewart/bitcoin/commit/04815c463d331f674f67fdcb3b8da601484d7a33
It appears that to _totally_ clear the orphanage, we need to propagate the parent transaction for the last child transaction, which is unexpected I believe?
💬 garlonicon commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2423211697)
I guess some ASIC miners still didn't upgrade their nodes:
```
$ getchaintips
...
{
"height": 50831,
"hash": "000000000060a808a1067bed2feb0b22261383bb0f9b6e3f0dee2436eb8e5048",
"branchlen": 432,
"status": "headers-only"
},
{
"height": 50826,
"hash": "000000000035411cdce9394ab799205c7dd0ce99fd23b23f9481e591635fd0f2",
"branchlen": 427,
"status": "headers-only"
},
{
"height": 50817,
"hash": "0000000073a34762fc9f48419a7fba80c177
...
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2423211697)
I guess some ASIC miners still didn't upgrade their nodes:
```
$ getchaintips
...
{
"height": 50831,
"hash": "000000000060a808a1067bed2feb0b22261383bb0f9b6e3f0dee2436eb8e5048",
"branchlen": 432,
"status": "headers-only"
},
{
"height": 50826,
"hash": "000000000035411cdce9394ab799205c7dd0ce99fd23b23f9481e591635fd0f2",
"branchlen": 427,
"status": "headers-only"
},
{
"height": 50817,
"hash": "0000000073a34762fc9f48419a7fba80c177
...