💬 l0rinc commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#discussion_r2416668248)
+1 (except the double space in the suggestion)
(https://github.com/bitcoin/bitcoin/pull/33573#discussion_r2416668248)
+1 (except the double space in the suggestion)
💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2416674361)
I have extracted it on purpose to signal that both branches are doing something similar and have the same dependencies
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2416674361)
I have extracted it on purpose to signal that both branches are doing something similar and have the same dependencies
💬 maflcko commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2416744379)
https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html says "the usage of string literals is idiomatic." But no strong opinion.
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2416744379)
https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html says "the usage of string literals is idiomatic." But no strong opinion.
💬 ryanofsky commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416741361)
re: https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956
Not important, but FWIW sjors suggestion makes a little more sense to me to I don't see sending unknown log messages to the trace stream, which is effectively a black hole, as being more conservative. IMO ideal thing to do would be to return `std::optional` and `nullopt` and then log unknown messages at debug level with a prefix like "[unrecognized flags "0x%08x]".
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416741361)
re: https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956
Not important, but FWIW sjors suggestion makes a little more sense to me to I don't see sending unknown log messages to the trace stream, which is effectively a black hole, as being more conservative. IMO ideal thing to do would be to return `std::optional` and `nullopt` and then log unknown messages at debug level with a prefix like "[unrecognized flags "0x%08x]".
💬 ryanofsky commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416732939)
re: https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2410969953
Yeah just a tradeoff between having extra variables and having an extra level a nesting. I think having more variables is worse, but no strong opinion and you should follow your preference
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416732939)
re: https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2410969953
Yeah just a tradeoff between having extra variables and having an extra level a nesting. I think having more variables is worse, but no strong opinion and you should follow your preference
👍 ryanofsky approved a pull request: "multiprocess: Fix high overhead from message logging"
(https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3318880294)
Code review ACK dcaf4b736f9ddfc6a2695ce19221a7f383e43e20. Just rebased onto #33518 and changed assert(false) to log trace and since last review
re: https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3313376282
> The former seems too chatty, the latter too quiet.
Good observations, created https://github.com/bitcoin-core/libmultiprocess/issues/227 to track this
(https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3318880294)
Code review ACK dcaf4b736f9ddfc6a2695ce19221a7f383e43e20. Just rebased onto #33518 and changed assert(false) to log trace and since last review
re: https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3313376282
> The former seems too chatty, the latter too quiet.
Good observations, created https://github.com/bitcoin-core/libmultiprocess/issues/227 to track this
🤔 brunoerg reviewed a pull request: "test: add functional test for `TestShell` (matching doc example)"
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3318966739)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3318966739)
Concept ACK
💬 jonbourne commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385887546)
Please do not release this version now. Reschedule and rethink. It has many people who disagree with choices made, and they have not been discussed enough to release the code. This is a very important turning point, and the implications are very difficult, if not impossible, to take back. Specifically, remove any code that touches OP_RETURN. Please do not release this version.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385887546)
Please do not release this version now. Reschedule and rethink. It has many people who disagree with choices made, and they have not been discussed enough to release the code. This is a very important turning point, and the implications are very difficult, if not impossible, to take back. Specifically, remove any code that touches OP_RETURN. Please do not release this version.
🤔 cedwies reviewed a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3318992246)
Tested ACK 44a7261
Unit/Functional tests pass.
Checked `git grep RelayTransaction`
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3318992246)
Tested ACK 44a7261
Unit/Functional tests pass.
Checked `git grep RelayTransaction`
💬 cedwies commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2416807372)
Nit: Maybe we should change this log message.
Something like
`LogPrint(BCLog::NET, "Force scheduling tx %s for broadcast\n", tx.GetHash().ToString());`
That wording would better match the new method name and clarifies that we are queuing it, not sending instantly.
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2416807372)
Nit: Maybe we should change this log message.
Something like
`LogPrint(BCLog::NET, "Force scheduling tx %s for broadcast\n", tx.GetHash().ToString());`
That wording would better match the new method name and clarifies that we are queuing it, not sending instantly.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416836217)
There are two reasons for this. One is that if you define a struct, the user has to instantiate that struct and copy the data to it instead of providing a pointer directly. This also introduces some questions around the lifetime of the data. I still think this function is straight forward enough to use. This might look clunky in C, but we don't expect many developers to use the C header directly. Language bindings can provide nicer ways to use it.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416836217)
There are two reasons for this. One is that if you define a struct, the user has to instantiate that struct and copy the data to it instead of providing a pointer directly. This also introduces some questions around the lifetime of the data. I still think this function is straight forward enough to use. This might look clunky in C, but we don't expect many developers to use the C header directly. Language bindings can provide nicer ways to use it.
💬 aimtiaz11 commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385946895)
Please let common sense prevail and avoid making this regrettable mistake of increasing OP_RETURN. It’s not too late.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385946895)
Please let common sense prevail and avoid making this regrettable mistake of increasing OP_RETURN. It’s not too late.
💬 naturallaw777 commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385975103)
Yes, do not release this version! The OP_RETURN limit increase will allow bad actors to harm the network in irrevocable ways.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385975103)
Yes, do not release this version! The OP_RETURN limit increase will allow bad actors to harm the network in irrevocable ways.
💬 l0rinc commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385993656)
@aimtiaz11, @naturallaw777: this isn't the place to discuss that topic, please read https://delvingbitcoin.org/t/response-to-pieter-wuilles-stackexchange-answer-re-nuking-the-opreturn-filter/1991/11 for a motivation for these changes and why it's just a mitigation for the problem. Feel free to respond there, but this topic is about the release schedule, let's keep it that way.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385993656)
@aimtiaz11, @naturallaw777: this isn't the place to discuss that topic, please read https://delvingbitcoin.org/t/response-to-pieter-wuilles-stackexchange-answer-re-nuking-the-opreturn-filter/1991/11 for a motivation for these changes and why it's just a mitigation for the problem. Feel free to respond there, but this topic is about the release schedule, let's keep it that way.
💬 pinheadmz commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385997286)
Off topic comments will be removed and users banned for at least one day.
See https://github.com/bitcoin-core/meta/discussions/18
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385997286)
Off topic comments will be removed and users banned for at least one day.
See https://github.com/bitcoin-core/meta/discussions/18
💬 theuni commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416893036)
By "more conservative", I meant from a Core logging DOS pov.
If libmp ever added a super-duper-verbose mode, imo the safer thing to do would be to black-hole those messages rather than fill the log with them. Sure that would hide issues, but it would also prevent accidental blowups. I don't see that as likely at all, just thinking defensively by default.
I could imagine sending a single "Unrecognized log level specified" warning, but that doesn't really seem worth the complexity for a theo
...
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416893036)
By "more conservative", I meant from a Core logging DOS pov.
If libmp ever added a super-duper-verbose mode, imo the safer thing to do would be to black-hole those messages rather than fill the log with them. Sure that would hide issues, but it would also prevent accidental blowups. I don't see that as likely at all, just thinking defensively by default.
I could imagine sending a single "Unrecognized log level specified" warning, but that doesn't really seem worth the complexity for a theo
...
💬 andrewtoth commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3386003121)
ACK 44a726133a880f818228c01b55236b3cb3eb1a67
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3386003121)
ACK 44a726133a880f818228c01b55236b3cb3eb1a67
💬 atanasna commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386006614)
Dear core, you have been a crucial part of bitcoin for many many years and we are all thankfull for all your efforts.
What you are doing with this update is not only degrading your own reputation(this is not an opinion but a fact proven by the 20+% of node operators moving to knots) but also putting at risk the single thing that can save us from the jaws of the tyrannical regime that the banks, fiat money and governments have systemically put us into.
Bitcoin will continue with or without yo
...
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386006614)
Dear core, you have been a crucial part of bitcoin for many many years and we are all thankfull for all your efforts.
What you are doing with this update is not only degrading your own reputation(this is not an opinion but a fact proven by the 20+% of node operators moving to knots) but also putting at risk the single thing that can save us from the jaws of the tyrannical regime that the banks, fiat money and governments have systemically put us into.
Bitcoin will continue with or without yo
...
💬 brunoerg commented on pull request "test: add functional test for `TestShell` (matching doc example)":
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2416904478)
After the shutdown, we could reset `test` and check if `num_nodes` has been reset. E.g.:
```diff
finally:
test.shutdown()
+ test.reset()
+ assert test.num_nodes is None
```
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2416904478)
After the shutdown, we could reset `test` and check if `num_nodes` has been reset. E.g.:
```diff
finally:
test.shutdown()
+ test.reset()
+ assert test.num_nodes is None
```
💬 Dorex45 commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386015649)
Candidate Release V30 ACK.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3386015649)
Candidate Release V30 ACK.