📝 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
...
📝 toogoodtofail opened a pull request: "Simple security.doc improvement"
(https://github.com/bitcoin/bitcoin/pull/31114)
Improvement on readability and usability of (preview) markdown code.
(https://github.com/bitcoin/bitcoin/pull/31114)
Improvement on readability and usability of (preview) markdown code.
💬 ariard commented on issue "Follow-up from Moderation Rules":
(https://github.com/bitcoin/bitcoin/issues/31110#issuecomment-2423252219)
> It speculates about individuals rather than focusing on ideas.
It’s not like some people with perm rights have actually done “fat fingers” mistakes in the past…( and I can go back in IRC logs here). If it’s an infrastructure compromise it’s still better to be verbose about it. Github fwiw is used for code distribution among the whole ecosystem. It’s very different than a bug directly affecting core code.
Better to continue the discussion in /meta.
(https://github.com/bitcoin/bitcoin/issues/31110#issuecomment-2423252219)
> It speculates about individuals rather than focusing on ideas.
It’s not like some people with perm rights have actually done “fat fingers” mistakes in the past…( and I can go back in IRC logs here). If it’s an infrastructure compromise it’s still better to be verbose about it. Github fwiw is used for code distribution among the whole ecosystem. It’s very different than a bug directly affecting core code.
Better to continue the discussion in /meta.
💬 andrewtoth commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2423256908)
While trying to implement the approach above, it became apparent that we still need the call to ReallocateCache here. This is for the case when IBD is done with memory normally reserved for the mempool. After IBD is finished and the mempool starts filling up and encroaching on the memory used by the cache, the cache must be reallocated. There is no other way to shrink it.
I think it's good that we are not calling reallocate on the ephemeral views, but I think we can do better with the preallo
...
(https://github.com/bitcoin/bitcoin/pull/30370#issuecomment-2423256908)
While trying to implement the approach above, it became apparent that we still need the call to ReallocateCache here. This is for the case when IBD is done with memory normally reserved for the mempool. After IBD is finished and the mempool starts filling up and encroaching on the memory used by the cache, the cache must be reallocated. There is no other way to shrink it.
I think it's good that we are not calling reallocate on the ephemeral views, but I think we can do better with the preallo
...
💬 Prabhat1308 commented on 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#issuecomment-2423265583)
Concept ACK for the test
While we are trying to add another orphan transaction when the max amount is reached , why are we dropping a random orphan and not just the incoming orphan ? Even in the condition of the node getting DOSed , having to remove a random transaction translates to some amount of work done while its not the case when we just drop the incoming transaction.
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2423265583)
Concept ACK for the test
While we are trying to add another orphan transaction when the max amount is reached , why are we dropping a random orphan and not just the incoming orphan ? Even in the condition of the node getting DOSed , having to remove a random transaction translates to some amount of work done while its not the case when we just drop the incoming transaction.
⚠️ Sandra-Amina-Boss opened an issue: "From https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer:"
(https://github.com/bitcoin/bitcoin/issues/31115)
From https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer:
> brew install llvm
...
> $ cmake --preset=libfuzzer -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" -DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
_Originally posted by @sipa in https://github.com/bitcoin/bitcoin/issues/31111#issuecomment-2421978663_
(https://github.com/bitcoin/bitcoin/issues/31115)
From https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer:
> brew install llvm
...
> $ cmake --preset=libfuzzer -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" -DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
_Originally posted by @sipa in https://github.com/bitcoin/bitcoin/issues/31111#issuecomment-2421978663_
✅ fanquake closed an issue: "From https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#macos-hints-for-libfuzzer:"
(https://github.com/bitcoin/bitcoin/issues/31115)
(https://github.com/bitcoin/bitcoin/issues/31115)
:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/31115)
(https://github.com/bitcoin/bitcoin/issues/31115)
💬 theStack commented on issue "COIN_VALUE no longer accessible in const contexts":
(https://github.com/bitcoin/bitcoin/issues/31113#issuecomment-2423360017)
Was this issue intended for another project? Bitcoin Core never had a `COIN_VALUE` constant, there is also no `Amount` class (or namespace).
(https://github.com/bitcoin/bitcoin/issues/31113#issuecomment-2423360017)
Was this issue intended for another project? Bitcoin Core never had a `COIN_VALUE` constant, there is also no `Amount` class (or namespace).