💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2722497291)
I just opened https://github.com/bitcoin/bitcoin/pull/32061 which consumes `Sockman` for HTTP. Thanks to @theuni help at coredev I'm now pretty happy with the performance. It obviously doesn't require a p2p refactor -- so we could just review sockman on its own, or in the context of HTTP
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2722497291)
I just opened https://github.com/bitcoin/bitcoin/pull/32061 which consumes `Sockman` for HTTP. Thanks to @theuni help at coredev I'm now pretty happy with the performance. It obviously doesn't require a p2p refactor -- so we could just review sockman on its own, or in the context of HTTP
📝 pinheadmz converted_to_draft a pull request: "[draft] Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
💬 brunoerg commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994209990)
I think linux is more permissive than macOS that's why CI is failing. The jobs do not set `-fprofile-instr-generate -fcoverage-mapping`. I did not test, but using lld could work?
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994209990)
I think linux is more permissive than macOS that's why CI is failing. The jobs do not set `-fprofile-instr-generate -fcoverage-mapping`. I did not test, but using lld could work?
👍 hodlinator approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2683225882)
re-ACK 565184e6e81a58c9cf60081cdced9a8d47725664
range-diff with [previous ACK](https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2661348106) confirmed only binary path fixups stemming from #31161 - happy that finally landed! :tada:
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2683225882)
re-ACK 565184e6e81a58c9cf60081cdced9a8d47725664
range-diff with [previous ACK](https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2661348106) confirmed only binary path fixups stemming from #31161 - happy that finally landed! :tada:
👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2683088562)
Code review ACK c107aa8c873cb3c031b341baef487638133823f1. Changes since last review were just updating some messages and expanding the test a little. Thanks for the updates. This still looks good to me and I left some more comments, but they are not important.
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2683088562)
Code review ACK c107aa8c873cb3c031b341baef487638133823f1. Changes since last review were just updating some messages and expanding the test a little. Thanks for the updates. This still looks good to me and I left some more comments, but they are not important.
💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994143659)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954056589
> Thanks! This made me actually realize I could add a test for it and improve the exception message (+ commit message).
I'm still not clear when this case would ever be expected to happen.
IIUC, there is no runtime error that could trigger this, and nothing non-test code could could do to trigger it, and the only thing that could trigger it is incorrectly written test code?
If so, it would seem clearer to replac
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994143659)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954056589
> Thanks! This made me actually realize I could add a test for it and improve the exception message (+ commit message).
I'm still not clear when this case would ever be expected to happen.
IIUC, there is no runtime error that could trigger this, and nothing non-test code could could do to trigger it, and the only thing that could trigger it is incorrectly written test code?
If so, it would seem clearer to replac
...
💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994186307)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953499371
> I think it is good to document _why_ we are catching and suppressing some exceptions (while re-raising others).
It's funny, because to me replacing "Initialization phase" with "Suppress these in favor of a later outcome" is literally *removing* the comment that explains why it is ok to suppress these errors. The previous comment made it clear these errors were being suppressed because they are expected during initia
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994186307)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953499371
> I think it is good to document _why_ we are catching and suppressing some exceptions (while re-raising others).
It's funny, because to me replacing "Initialization phase" with "Suppress these in favor of a later outcome" is literally *removing* the comment that explains why it is ok to suppress these errors. The previous comment made it clear these errors were being suppressed because they are expected during initia
...
💬 jonatack commented on pull request "[IBD] flush UXTOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2722607551)
Review and testing-under-way ACK 868413340f8d6058d74186b65ac3498d6b7f254a
As described in the pull description, this value was set in 2017 in #10148 and hasn't been changed since.
> Maybe a config option can be provided to change it.
> Just realized [`dbbatchsize`](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/init.cpp#L489) already exists.
This is a hidden config option and the rationale might be https://github.com/bitcoin/bitcoin/pull/10148#di
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2722607551)
Review and testing-under-way ACK 868413340f8d6058d74186b65ac3498d6b7f254a
As described in the pull description, this value was set in 2017 in #10148 and hasn't been changed since.
> Maybe a config option can be provided to change it.
> Just realized [`dbbatchsize`](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/init.cpp#L489) already exists.
This is a hidden config option and the rationale might be https://github.com/bitcoin/bitcoin/pull/10148#di
...
🚀 ryanofsky merged a pull request: "qa: clarify and document one assumeutxo test case with malleated snapshot"
(https://github.com/bitcoin/bitcoin/pull/31907)
(https://github.com/bitcoin/bitcoin/pull/31907)
💬 ryanofsky commented on pull request "qa: clarify and document one assumeutxo test case with malleated snapshot":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722632443)
Merged since it was requested in IRC. Could be useful to follow up on comments from https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2661853159 though
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722632443)
Merged since it was requested in IRC. Could be useful to follow up on comments from https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2661853159 though
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722641649)
> I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior.
Yes, I think I have come to a similar conclusion, and I think the introduced reliance here on the `m_target_blockhash` and `m_from_snapshot_blockhash` are very good changes. Having some kind of "start" and "stop" blocks for a chainstate seems like something that is easy to understand and might also be useful beyond a
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722641649)
> I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior.
Yes, I think I have come to a similar conclusion, and I think the introduced reliance here on the `m_target_blockhash` and `m_from_snapshot_blockhash` are very good changes. Having some kind of "start" and "stop" blocks for a chainstate seems like something that is easy to understand and might also be useful beyond a
...
💬 TheCharlatan commented on pull request "qa: clarify and document one assumeutxo test case with malleated snapshot":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722677016)
Post-merge ACK
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722677016)
Post-merge ACK
🚀 glozow merged a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049)
(https://github.com/bitcoin/bitcoin/pull/32049)
🤔 furszy reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2683350728)
Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2683350728)
Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
💬 furszy commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1994294882)
tiny nit: could provide a less aggressive `check_interval`. Like 0.1.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1994294882)
tiny nit: could provide a less aggressive `check_interval`. Like 0.1.
📝 glozow opened a pull request: "[29.x] backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/32062)
Placeholder, collecting backports + anything else for rc2
- #32049
(https://github.com/bitcoin/bitcoin/pull/32062)
Placeholder, collecting backports + anything else for rc2
- #32049
💬 darosior commented on pull request "test: avoid treating hash results as integers":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722704531)
The concept of this PR is appealing but i don't think it's true that we don't usually rely on hashes being numbers. I think more often than not we do, as we have a bunch of ordered indexes keyed by hashes. A good example of this is the mempool's `mapTx`:
https://github.com/bitcoin/bitcoin/blob/5c2f04413e49545cbefcd232f9c1b6bd08688899/src/txmempool.h#L245-L250
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722704531)
The concept of this PR is appealing but i don't think it's true that we don't usually rely on hashes being numbers. I think more often than not we do, as we have a bunch of ordered indexes keyed by hashes. A good example of this is the mempool's `mapTx`:
https://github.com/bitcoin/bitcoin/blob/5c2f04413e49545cbefcd232f9c1b6bd08688899/src/txmempool.h#L245-L250
👍 TheCharlatan approved a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2683379972)
ACK dfd0a76fc1b46ddd1f4dab388ec2ca7fc3f76a2b
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2683379972)
ACK dfd0a76fc1b46ddd1f4dab388ec2ca7fc3f76a2b
💬 TheCharlatan commented on pull request "test: Check datadir cleanup after assumeutxo was successful":
(https://github.com/bitcoin/bitcoin/pull/32033#discussion_r1994314303)
Nit: Might it be worthwhile to also check existence beforehand to guard against naming changes, or the behaviour of the indexed nodes changing?
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 89cf703fce..2166c4505d 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -645,0 +646,2 @@ class AssumeutxoTest(BitcoinTestFramework):
+ if i != 0:
+ assert (n.datadir_pa
...
(https://github.com/bitcoin/bitcoin/pull/32033#discussion_r1994314303)
Nit: Might it be worthwhile to also check existence beforehand to guard against naming changes, or the behaviour of the indexed nodes changing?
```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index 89cf703fce..2166c4505d 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -645,0 +646,2 @@ class AssumeutxoTest(BitcoinTestFramework):
+ if i != 0:
+ assert (n.datadir_pa
...
💬 theuni commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2722723839)
@fanquake Can you describe why you think it's important that they align? I think they're 2 different concerns, really. It seems like it'd be a pretty rare circumstance where we'd need depends compiled with max gdb debugability, but pretty common that we'd want Core built that way.
Aligning depends and Core builds both at `-Og` seemed reasonable to me, but @achow101 has a compelling argument against that. I'm not so excited about both at `-O0` because that's basically _only_ useful for debuggi
...
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2722723839)
@fanquake Can you describe why you think it's important that they align? I think they're 2 different concerns, really. It seems like it'd be a pretty rare circumstance where we'd need depends compiled with max gdb debugability, but pretty common that we'd want Core built that way.
Aligning depends and Core builds both at `-Og` seemed reasonable to me, but @achow101 has a compelling argument against that. I'm not so excited about both at `-O0` because that's basically _only_ useful for debuggi
...