💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476580792)
I [wrote](https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141986299):
> using `warning` for now seems fine, since we don't actually print the word "warning" in the log.
I should have tested that, because it actually _does_ print `[validation:warning]` in the log.
Maybe we should just stick to `fprintf` and change the TODO to say this should be `validation` level `info` once `LogPrintf` prints those by default.
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476580792)
I [wrote](https://github.com/bitcoin/bitcoin/pull/27278#discussion_r1141986299):
> using `warning` for now seems fine, since we don't actually print the word "warning" in the log.
I should have tested that, because it actually _does_ print `[validation:warning]` in the log.
Maybe we should just stick to `fprintf` and change the TODO to say this should be `validation` level `info` once `LogPrintf` prints those by default.
🚀 achow101 merged a pull request: "mempool: Add mempool tracepoints"
(https://github.com/bitcoin/bitcoin/pull/26531)
(https://github.com/bitcoin/bitcoin/pull/26531)
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142420455)
Sorry, forgot to remove all that.
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142420455)
Sorry, forgot to remove all that.
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142421537)
Was unintentional, going to place it on its original location.
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142421537)
Was unintentional, going to place it on its original location.
🚀 achow101 merged a pull request: "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`"
(https://github.com/bitcoin/bitcoin/pull/26899)
(https://github.com/bitcoin/bitcoin/pull/26899)
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142422788)
ok
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142422788)
ok
🚀 achow101 merged a pull request: "guix: use osslsigncode 2.5"
(https://github.com/bitcoin/bitcoin/pull/27179)
(https://github.com/bitcoin/bitcoin/pull/27179)
💬 achow101 commented on pull request "rpc: In `utxoupdatepsbt` also look for the tx in the txindex":
(https://github.com/bitcoin/bitcoin/pull/25939#issuecomment-1476594222)
ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98
(https://github.com/bitcoin/bitcoin/pull/25939#issuecomment-1476594222)
ACK 6e9f8bb050785dbc754b6bb493aad9139908ef98
💬 TheCharlatan commented on pull request "ci: Cache more stuff in the ci images: msan, iwyu, pip, sdks":
(https://github.com/bitcoin/bitcoin/pull/27028#issuecomment-1476594464)
Light code review ACK 5555a336019848d2e19e4533b3d2f34e438e6367
Confirmed that it indeed caches these additional packages. That said, I am not familiar with the build flow through the various scripts and cannot really comment if the code was moved to the correct place, or if conventions were followed.
> Would be good to make progress here.
Yes, I need to get a better scheduling workflow for PR's requiring interaction.
(https://github.com/bitcoin/bitcoin/pull/27028#issuecomment-1476594464)
Light code review ACK 5555a336019848d2e19e4533b3d2f34e438e6367
Confirmed that it indeed caches these additional packages. That said, I am not familiar with the build flow through the various scripts and cannot really comment if the code was moved to the correct place, or if conventions were followed.
> Would be good to make progress here.
Yes, I need to get a better scheduling workflow for PR's requiring interaction.
💬 fanquake commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476598738)
NACK. This is pretty literally the opposite of what we want to be doing with the kernel.
> And in master branch libbitcoinkernel_la_SOURCES and libbitcoin_util_a_SOURCES have almost the same source file lists.
The while point of having these files enumerated, is so that we know what is currently in the kernel, and can continue to (easily) prune deps, while refactoring. Hiding this away into `libbitcoin_util` only makes this harder/impossible to do, and you're just removing the current, exp
...
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476598738)
NACK. This is pretty literally the opposite of what we want to be doing with the kernel.
> And in master branch libbitcoinkernel_la_SOURCES and libbitcoin_util_a_SOURCES have almost the same source file lists.
The while point of having these files enumerated, is so that we know what is currently in the kernel, and can continue to (easily) prune deps, while refactoring. Hiding this away into `libbitcoin_util` only makes this harder/impossible to do, and you're just removing the current, exp
...
💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142432253)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142432253)
Fixed
💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142432364)
Done
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142432364)
Done
💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142432481)
Done
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1142432481)
Done
💬 Sjors commented on pull request "Log new headers":
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476604650)
That said, it seems quite trivial to just change the default log level: https://github.com/Sjors/bitcoin/commit/0ecb8290eeffcd2a2172c6720a2168404870530b
So far that's not generating noise for me.
(https://github.com/bitcoin/bitcoin/pull/27278#issuecomment-1476604650)
That said, it seems quite trivial to just change the default log level: https://github.com/Sjors/bitcoin/commit/0ecb8290eeffcd2a2172c6720a2168404870530b
So far that's not generating noise for me.
💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1476608456)
force push to: 93c70287a6434c6c665a211dc4dfbbd9c3db4083
- rebase on master
- fix failing unit test
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1476608456)
force push to: 93c70287a6434c6c665a211dc4dfbbd9c3db4083
- rebase on master
- fix failing unit test
💬 brunoerg commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142437445)
nit:
```suggestion
const std::string uri{"localhost:8080/rest/headers/someresource.json"};
```
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142437445)
nit:
```suggestion
const std::string uri{"localhost:8080/rest/headers/someresource.json"};
```
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142439278)
right
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142439278)
right
💬 TheCharlatan commented on pull request "build: Link `libbitcoinkernel` to `libbitcoin_util`":
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476617574)
Concept NACK
> Isn't libbitcoin_util supposed to be a reusable build artifact?
Maybe, but linking in the entirety of the util library is not what the kernel project is currently trying to achieve. The goal is to prune as many files as possible out of the library. This also stated in stage 1 step 2 of the original kernel issue: https://github.com/bitcoin/bitcoin/issues/24303 . Once stage 2 is reached, there may be a discussion again of a shared build artifact containing the intersection of
...
(https://github.com/bitcoin/bitcoin/pull/27285#issuecomment-1476617574)
Concept NACK
> Isn't libbitcoin_util supposed to be a reusable build artifact?
Maybe, but linking in the entirety of the util library is not what the kernel project is currently trying to achieve. The goal is to prune as many files as possible out of the library. This also stated in stage 1 step 2 of the original kernel issue: https://github.com/bitcoin/bitcoin/issues/24303 . Once stage 2 is reached, there may be a discussion again of a shared build artifact containing the intersection of
...
✅ hebasto closed a pull request: "build: Link `libbitcoinkernel` to `libbitcoin_util`"
(https://github.com/bitcoin/bitcoin/pull/27285)
(https://github.com/bitcoin/bitcoin/pull/27285)
💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142447667)
I wanted to make sure then to use the free up function for the request in the `HTTPRequest` destructor: `evhttp_request_free`. This was something I was doing while investigating the previous failure of the ASan tests and possible memory leaks. I'll remove it if you think unnecessary,
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1142447667)
I wanted to make sure then to use the free up function for the request in the `HTTPRequest` destructor: `evhttp_request_free`. This was something I was doing while investigating the previous failure of the ASan tests and possible memory leaks. I'll remove it if you think unnecessary,