💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790649002)
Done.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790649002)
Done.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790647190)
I have phrased it as "this information has to be inferred from the ancestor sets" (and similar for `GetReducedChildren`).
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790647190)
I have phrased it as "this information has to be inferred from the ancestor sets" (and similar for `GetReducedChildren`).
💬 ismaelsadeeq commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790656735)
much better 👍🏾
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1790656735)
much better 👍🏾
🤔 pablomartin4btc reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2352608848)
tACK dae1d48c546c2e1daadbf982420ba3d8feda2a9f
Tested on Ubuntu 22.04 using `-DCMAKE_BUILD_TYPE=Debug` work fine.
With `depends`:
`-- Found Qt: /home/pablo/workspace/bitcoin/depends/x86_64-pc-linux-gnu/lib/cmake/Qt6 (found suitable version "6.7.3", minimum required is "6.2") `
In order to run `bitcoin-qt` I had to install `libxcb-cursor0` (as expected specified in the description). Is this going to be clarified somewhere?
tACK dae1d48c546c2e1daadbf982420ba3d8feda2a9f
Tested on Ubuntu 22.04 using `-DCMAKE_BUILD_TYPE=Debug` work fine.
With `depends`:
`-- Found Qt: /home/pablo/workspace/bitcoin/depends/x86_64-pc-linux-gnu/lib/cmake/Qt6 (found suitable version "6.7.3", minimum required is "6.2") `
In order to run `bitcoin-qt` I had to install `libxcb-cursor0` (as expected specified in the description). Is this going to be clarified somewhere?
![image](https://github.com/user-attachments/assets/000109cd
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790666758)
What if the peer doesn't answer our ping (because they are broken / malicious)? I think we would never disconnect them because the disconnection logic is inside of `MaybeSendPing()`, which will never be called again for private peers.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790666758)
What if the peer doesn't answer our ping (because they are broken / malicious)? I think we would never disconnect them because the disconnection logic is inside of `MaybeSendPing()`, which will never be called again for private peers.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790700993)
> Which one?
No strong opinion (I think that both should work), but the second one (also check wtxid) seems simpler.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790700993)
> Which one?
No strong opinion (I think that both should work), but the second one (also check wtxid) seems simpler.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790649950)
The final fixup commit leads to the question what should happen if the peer doesn't send a GETDATA to our INV. I think this would happen in practice if they are one of the later broadcast attempts and already received the tx from someone else (as a result of our first attempts). I think that we would currently never disconnect on our end, but they might disconnect us after `TIMEOUT_INTERVAL=20min` because we don't answer their ping. But if they don't have this logic the connection might stay ope
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790649950)
The final fixup commit leads to the question what should happen if the peer doesn't send a GETDATA to our INV. I think this would happen in practice if they are one of the later broadcast attempts and already received the tx from someone else (as a result of our first attempts). I think that we would currently never disconnect on our end, but they might disconnect us after `TIMEOUT_INTERVAL=20min` because we don't answer their ping. But if they don't have this logic the connection might stay ope
...
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790699672)
I didn't mean to suggest a change to the logic, just to make it more conditional on `-privatebroadcast`, so that in principle we wouldn't even need to initiate a `m_private_broadcast` object unless the user is running in `-privatebroadcast` mode. But I don't have a concrete suggestion right now (I might suggest on at a later point but need to look deeper into the grant logic for that) so it's fine to resolve for now.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1790699672)
I didn't mean to suggest a change to the logic, just to make it more conditional on `-privatebroadcast`, so that in principle we wouldn't even need to initiate a `m_private_broadcast` object unless the user is running in `-privatebroadcast` mode. But I don't have a concrete suggestion right now (I might suggest on at a later point but need to look deeper into the grant logic for that) so it's fine to resolve for now.
👍 BrandonOdiwuor approved a pull request: "test: remove unused code from `script_tests`"
(https://github.com/bitcoin/bitcoin/pull/31051#pullrequestreview-2352705365)
ACK e0287bc4b2d83cefb7af48aef457153d0729bcd4
(https://github.com/bitcoin/bitcoin/pull/31051#pullrequestreview-2352705365)
ACK e0287bc4b2d83cefb7af48aef457153d0729bcd4
💬 fanquake commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2397658116)
Hitting the 20m `--timeout` https://cirrus-ci.com/task/5886749702881280:
```bash
[17:40:38.816] 129/136 Test #135: walletload_tests ..................... Passed 4.68 sec
[17:40:42.991] 130/136 Test #132: wallet_tests ......................... Passed 11.89 sec
[17:40:47.154] 131/136 Test #122: coinselector_tests ................... Passed 23.22 sec
[17:40:54.153] 132/136 Test #1: util_test_runner ..................... Passed 119.52 sec
[17:41:12.100] 133/136 Test #5: nov
...
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2397658116)
Hitting the 20m `--timeout` https://cirrus-ci.com/task/5886749702881280:
```bash
[17:40:38.816] 129/136 Test #135: walletload_tests ..................... Passed 4.68 sec
[17:40:42.991] 130/136 Test #132: wallet_tests ......................... Passed 11.89 sec
[17:40:47.154] 131/136 Test #122: coinselector_tests ................... Passed 23.22 sec
[17:40:54.153] 132/136 Test #1: util_test_runner ..................... Passed 119.52 sec
[17:41:12.100] 133/136 Test #5: nov
...
💬 pablomartin4btc commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2397799341)
> > Ninja is required to build the `qt` package in depends. It is mentioned in `depends/README.md`. However, `doc/build-osx.md` does not cover building with depends, so I don't think it is necessary to mention Ninja there.
>
> Could you please explain why this is the case? Ninja seems like an odd requirement here.
It's described in the Qt 6/ 6.7 [dependencies](https://doc.qt.io/qt-6/integrity-installing-dependencies.html). I've tested it myself and can't build `depends` without `ninja-buil
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2397799341)
> > Ninja is required to build the `qt` package in depends. It is mentioned in `depends/README.md`. However, `doc/build-osx.md` does not cover building with depends, so I don't think it is necessary to mention Ninja there.
>
> Could you please explain why this is the case? Ninja seems like an odd requirement here.
It's described in the Qt 6/ 6.7 [dependencies](https://doc.qt.io/qt-6/integrity-installing-dependencies.html). I've tested it myself and can't build `depends` without `ninja-buil
...
💬 furszy commented on pull request "Decouple WalletModel from RPCExecutor":
(https://github.com/bitcoin-core/gui/pull/841#issuecomment-2397806974)
> Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the settings.json file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn't correspond with the one in the main window, which is ok if you want to compare data but I think when the console opens it should have the same. Also perhaps we should show the last used wallet (in ./config/Bitcoin-Qt-*.conf?) when qt starts (if st
...
(https://github.com/bitcoin-core/gui/pull/841#issuecomment-2397806974)
> Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the settings.json file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn't correspond with the one in the main window, which is ok if you want to compare data but I think when the console opens it should have the same. Also perhaps we should show the last used wallet (in ./config/Bitcoin-Qt-*.conf?) when qt starts (if st
...
💬 pablomartin4btc commented on pull request "Decouple WalletModel from RPCExecutor":
(https://github.com/bitcoin-core/gui/pull/841#issuecomment-2397822725)
> If the goal is still to move to a new GUI, I wouldn't start changing the views behavior now.
Yeah, neither do I, just mentioning it here since I just passed thru while testing.
(https://github.com/bitcoin-core/gui/pull/841#issuecomment-2397822725)
> If the goal is still to move to a new GUI, I wouldn't start changing the views behavior now.
Yeah, neither do I, just mentioning it here since I just passed thru while testing.
🤔 pablomartin4btc reviewed a pull request: "build: Rename `PACKAGE_*` variables to `CLIENT_*`"
(https://github.com/bitcoin/bitcoin/pull/31042#pullrequestreview-2352879220)
ACK c87a9d2e4c0ad7f2653857f34d7e048585f00be2
(https://github.com/bitcoin/bitcoin/pull/31042#pullrequestreview-2352879220)
ACK c87a9d2e4c0ad7f2653857f34d7e048585f00be2
🤔 mzumsande reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2352892509)
Conceptual question:
I think that just the goal of not loading the blocktreedb twice could have been achieved easier by simply moving the code from `CompleteChainstateInitialization` into `LoadChainstate()`.
You also list other motivations, but I don't completely get these:
It seems a little bit non-intuitive to me to have things like Cleanup of blk / rev files (`CleanupBlockRevFiles`) done in the constructor, instead of having a separate call for this.
It also doesn't feel consistent if
...
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2352892509)
Conceptual question:
I think that just the goal of not loading the blocktreedb twice could have been achieved easier by simply moving the code from `CompleteChainstateInitialization` into `LoadChainstate()`.
You also list other motivations, but I don't completely get these:
It seems a little bit non-intuitive to me to have things like Cleanup of blk / rev files (`CleanupBlockRevFiles`) done in the constructor, instead of having a separate call for this.
It also doesn't feel consistent if
...
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2397918937)
> Also playing around with this and reading the [netdriver post](https://blog.swiecki.net/2018/01/fuzzing-tcp-servers.html) (i couldn't find any other docs on it) I'm not sure if this will ever even get past the version handshake? So I'm wondering how useful this tool really is for us.
Yes, I don't think it will pass the version handshake. I can't see what kind of bugs it would find that other fuzzers would not get.
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2397918937)
> Also playing around with this and reading the [netdriver post](https://blog.swiecki.net/2018/01/fuzzing-tcp-servers.html) (i couldn't find any other docs on it) I'm not sure if this will ever even get past the version handshake? So I'm wondering how useful this tool really is for us.
Yes, I don't think it will pass the version handshake. I can't see what kind of bugs it would find that other fuzzers would not get.
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2397922175)
Re https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2352892509
Thank you for the questions @mzumsande!
> So can you explain a bit more in detail how this helps kernel?
I will try to give a thought-dump of what I am attempting to achieve here. As said in the title description, the kernel-motivated goal of this PR is to allow users of the library to instantiate the block tree db in the block manager in the same way we would currently in the chainstate load routines without
...
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2397922175)
Re https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2352892509
Thank you for the questions @mzumsande!
> So can you explain a bit more in detail how this helps kernel?
I will try to give a thought-dump of what I am attempting to achieve here. As said in the title description, the kernel-motivated goal of this PR is to allow users of the library to instantiate the block tree db in the block manager in the same way we would currently in the chainstate load routines without
...
📝 theuni opened a pull request: "RFC: build: support for pre-compiled headers."
(https://github.com/bitcoin/bitcoin/pull/31053)
This cuts off roughly a third of my total build time, either with `-j8` or `-j24`.
There are many different ways to go about adding pre-compiled header support with CMake. This is probably the simplest and most naive, but it has a substantial impact.
I used [ClangBuildAnalyzer](https://github.com/aras-p/ClangBuildAnalyzer) to measure the headers which took the most amount of time to parse while building the kernel library. I did this under the assumption that those are probably the most in
...
(https://github.com/bitcoin/bitcoin/pull/31053)
This cuts off roughly a third of my total build time, either with `-j8` or `-j24`.
There are many different ways to go about adding pre-compiled header support with CMake. This is probably the simplest and most naive, but it has a substantial impact.
I used [ClangBuildAnalyzer](https://github.com/aras-p/ClangBuildAnalyzer) to measure the headers which took the most amount of time to parse while building the kernel library. I did this under the assumption that those are probably the most in
...
💬 theuni commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2397943775)
Ping @hebasto, @fanquake
Also @willcl-ark, you might be interested in this as you've been benchmarking build times.
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2397943775)
Ping @hebasto, @fanquake
Also @willcl-ark, you might be interested in this as you've been benchmarking build times.
💬 TheCharlatan commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2397951718)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2397951718)
Concept ACK