💬 achow101 commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#issuecomment-2204494467)
ACK a6821a0a4e1cf2aba679962289f4beeefd3d539a
(https://github.com/bitcoin/bitcoin/pull/30272#issuecomment-2204494467)
ACK a6821a0a4e1cf2aba679962289f4beeefd3d539a
💬 achow101 commented on pull request "doc: use TRUC instead of v3 and add release note":
(https://github.com/bitcoin/bitcoin/pull/30272#issuecomment-2204502009)
ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
(https://github.com/bitcoin/bitcoin/pull/30272#issuecomment-2204502009)
ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
💬 achow101 commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2204502692)
ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2204502692)
ACK 2342b46c451658a418f8e28e50b2ad0e5abd284d
🚀 achow101 merged a pull request: "doc: use TRUC instead of v3 and add release note"
(https://github.com/bitcoin/bitcoin/pull/30272)
(https://github.com/bitcoin/bitcoin/pull/30272)
✅ achow101 closed an issue: "assumeutxo: nTx and nChainTx in RPC results are off"
(https://github.com/bitcoin/bitcoin/issues/29328)
(https://github.com/bitcoin/bitcoin/issues/29328)
🚀 achow101 merged a pull request: "rpc: Avoid getchaintxstats invalid results"
(https://github.com/bitcoin/bitcoin/pull/29720)
(https://github.com/bitcoin/bitcoin/pull/29720)
💬 Roasbeef commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1663344523)
> It's not clear to me why that is a problem though.
It gives those involved in the creation of the new parameters months of mining before the "public", which will skew the distribution of new testnet coins. If those that mined the chain for months before public release aren't also running faucets to easily disseminate the coins, then we end up in the same position where testnet coins are hard to obtain by developers.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1663344523)
> It's not clear to me why that is a problem though.
It gives those involved in the creation of the new parameters months of mining before the "public", which will skew the distribution of new testnet coins. If those that mined the chain for months before public release aren't also running faucets to easily disseminate the coins, then we end up in the same position where testnet coins are hard to obtain by developers.
🤔 tdb3 reviewed a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2155193780)
Approach ACK
Good work increasing clarity. In the past I've had a head-scratching moment with some of these before I learned the nuances, so this should help people.
Left a comment about release notes.
Tested:
- Ran unit/functional tests locally (passed)
- Manually exercised the following scenario (regtest):
- Created a transaction (and broadcasted).
- Generated one block to confirm.
- Attempted to call `sendrawtransaction` with the newly confirmed transaction.
- Receiv
...
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2155193780)
Approach ACK
Good work increasing clarity. In the past I've had a head-scratching moment with some of these before I learned the nuances, so this should help people.
Left a comment about release notes.
Tested:
- Ran unit/functional tests locally (passed)
- Manually exercised the following scenario (regtest):
- Created a transaction (and broadcasted).
- Generated one block to confirm.
- Attempted to call `sendrawtransaction` with the newly confirmed transaction.
- Receiv
...
💬 tdb3 commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663380344)
Although it's only the error message (and the number stays the same), I still think its appropriate add a release note for the change to the user.
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes
> Release notes should be written for any PR that:
> ...
> makes any other visible change to the end-user experience.
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663380344)
Although it's only the error message (and the number stays the same), I still think its appropriate add a release note for the change to the user.
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes
> Release notes should be written for any PR that:
> ...
> makes any other visible change to the end-user experience.
💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1663407636)
Thank you for catching this!
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1663407636)
Thank you for catching this!
💬 ajtowns commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2205008738)
> Thanks AJ, it's great to get specific and find out what actual problems you are concerned about,
What I'm concerned about is making logging unnecessarily complicated to use -- it should be easy to use, and it should be easy to understand what's going on without needing any context from the rest of the module where the logging is happening, or from debates about how logging should work.
You asked about specific problems this change could cause; and I'm happy to give examples, but they'r
...
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2205008738)
> Thanks AJ, it's great to get specific and find out what actual problems you are concerned about,
What I'm concerned about is making logging unnecessarily complicated to use -- it should be easy to use, and it should be easy to understand what's going on without needing any context from the rest of the module where the logging is happening, or from debates about how logging should work.
You asked about specific problems this change could cause; and I'm happy to give examples, but they'r
...
💬 TheCharlatan commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205252653)
Re https://github.com/bitcoin/bitcoin/pull/30371#issue-2384114654
> Locally, I can find the bug with -max_len=84 -use_value_profile=1 on a single thread on a laptop.
I've been running this for a day and did not find the crash. How long was this running for?
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205252653)
Re https://github.com/bitcoin/bitcoin/pull/30371#issue-2384114654
> Locally, I can find the bug with -max_len=84 -use_value_profile=1 on a single thread on a laptop.
I've been running this for a day and did not find the crash. How long was this running for?
💬 maflcko commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205271688)
16kk iterations; N=1. Again, I guess my Laptop just got extremely lucky.
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205271688)
16kk iterations; N=1. Again, I guess my Laptop just got extremely lucky.
📝 maflcko converted_to_draft a pull request: "fuzz: Mutate -max_len= during generation phase"
(https://github.com/bitcoin/bitcoin/pull/30371)
This revives https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1927243781, because it helps to find https://github.com/bitcoin/bitcoin/issues/30367, which failed to be found for more than a year on any of the existing fuzz servers.
Locally, I can find the bug with `-max_len=84 -use_value_profile=1` on a single thread on a laptop. The reason is likely that a smaller max_len results in a faster fuzzing speed (iterations). As a side-effect it may also be effective at reducing the size o
...
(https://github.com/bitcoin/bitcoin/pull/30371)
This revives https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1927243781, because it helps to find https://github.com/bitcoin/bitcoin/issues/30367, which failed to be found for more than a year on any of the existing fuzz servers.
Locally, I can find the bug with `-max_len=84 -use_value_profile=1` on a single thread on a laptop. The reason is likely that a smaller max_len results in a faster fuzzing speed (iterations). As a side-effect it may also be effective at reducing the size o
...
💬 maflcko commented on pull request "fuzz: Mutate -max_len= during generation phase":
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205276132)
Turning into draft for now, to allow for more time to benchmark and evaluate this.
(https://github.com/bitcoin/bitcoin/pull/30371#issuecomment-2205276132)
Turning into draft for now, to allow for more time to benchmark and evaluate this.
🤔 maflcko reviewed a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2155603386)
post-merge lgtm. Left some nits
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2155603386)
post-merge lgtm. Left some nits
💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194)
What is the point of translating this message when the translation is discarded? Seems wasteful of translator's time, no?
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194)
What is the point of translating this message when the translation is discarded? Seems wasteful of translator's time, no?
💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973)
It is not possible to pass a raw c-string pointer to transifex for translation. Only string literals can be translated.
I guess it could make sense to mark `_(const char*)` as `consteval` to catch those issues at compile time?
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973)
It is not possible to pass a raw c-string pointer to transifex for translation. Only string literals can be translated.
I guess it could make sense to mark `_(const char*)` as `consteval` to catch those issues at compile time?
💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663647981)
Is the trailing newline needed? This would be the first `JSONRPCError` with a trailing newline.
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663647981)
Is the trailing newline needed? This would be the first `JSONRPCError` with a trailing newline.
💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663637823)
Also, this commit is not a "refactor", because it changes logging and RPC error strings slightly.
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663637823)
Also, this commit is not a "refactor", because it changes logging and RPC error strings slightly.