💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190136977)
`CZMQPublishRawBlockNotifier` takes `get_block_by_index` by value (meaning instead of borrowing, copies its value), so I think that is why this works out.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190136977)
`CZMQPublishRawBlockNotifier` takes `get_block_by_index` by value (meaning instead of borrowing, copies its value), so I think that is why this works out.
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542522098)
Updated 886a473fc48f2c7d67436b5d9ac5643cd007e27f -> e3311d649d9785ca7bc3a29ad7470c7cd69951bb ([removeBlockstorageArgs_24](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_24) -> [removeBlockstorageArgs_25](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_25), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_24..removeBlockstorageArgs_25))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discuss
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542522098)
Updated 886a473fc48f2c7d67436b5d9ac5643cd007e27f -> e3311d649d9785ca7bc3a29ad7470c7cd69951bb ([removeBlockstorageArgs_24](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_24) -> [removeBlockstorageArgs_25](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_25), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_24..removeBlockstorageArgs_25))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discuss
...
🚀 achow101 merged a pull request: "refactor: Replace global find_value function with UniValue::find_value method"
(https://github.com/bitcoin/bitcoin/pull/27605)
(https://github.com/bitcoin/bitcoin/pull/27605)
💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190102588)
In commit "zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier" (6247b3c5e3c9e70097243a85b3d883b279a473cc)
Would probably be a little more efficient to make the get_block_by_index non-const and use `m_get_block_by_index{std::move(get_block_by_index)}` to move the function instead of copying it.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190102588)
In commit "zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier" (6247b3c5e3c9e70097243a85b3d883b279a473cc)
Would probably be a little more efficient to make the get_block_by_index non-const and use `m_get_block_by_index{std::move(get_block_by_index)}` to move the function instead of copying it.
👍 ryanofsky approved a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1420938880)
Code review ACK 886a473fc48f2c7d67436b5d9ac5643cd007e27f
re: https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542522098
> * Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934), moving the lambda instead of borrowing.
Actually I think my suggestion won't work. There can be multiple notifiers, so the function needs to be copied into all of them not moved into the first one. ACKed previous version of the PR 886a473fc48f2c7d6
...
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1420938880)
Code review ACK 886a473fc48f2c7d67436b5d9ac5643cd007e27f
re: https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542522098
> * Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934), moving the lambda instead of borrowing.
Actually I think my suggestion won't work. There can be multiple notifiers, so the function needs to be copied into all of them not moved into the first one. ACKed previous version of the PR 886a473fc48f2c7d6
...
💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190113760)
In commit "refactor/iwyu: Complete includes for blockmanger_args" (c065e093b1c786476d9bc9769e42c2a54ff68816)
"blockmanger_args" spelling in commit title
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190113760)
In commit "refactor/iwyu: Complete includes for blockmanger_args" (c065e093b1c786476d9bc9769e42c2a54ff68816)
"blockmanger_args" spelling in commit title
💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190174923)
re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934
Yeah sorry, I saw the "factories" map and assumed the factories would be long-lived, not destroyed before the function returns. This code is pretty strange, because it creates a map and then never looks anything up in the map. Could be cleaned up later, though. Probably a more sane way to write it would be to drop the map, change the outer for loop into an `add_notifiers` lambda, and do
```c++
add_notifiers("pubhashb
...
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190174923)
re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934
Yeah sorry, I saw the "factories" map and assumed the factories would be long-lived, not destroyed before the function returns. This code is pretty strange, because it creates a map and then never looks anything up in the map. Could be cleaned up later, though. Probably a more sane way to write it would be to drop the map, change the outer for loop into an `add_notifiers` lambda, and do
```c++
add_notifiers("pubhashb
...
💬 fanquake commented on issue "GCC 13: `-Wdangling-reference` output":
(https://github.com/bitcoin/bitcoin/issues/26926#issuecomment-1542538866)
Closing, now that #27605 has been merged.
(https://github.com/bitcoin/bitcoin/issues/26926#issuecomment-1542538866)
Closing, now that #27605 has been merged.
✅ fanquake closed an issue: "GCC 13: `-Wdangling-reference` output"
(https://github.com/bitcoin/bitcoin/issues/26926)
(https://github.com/bitcoin/bitcoin/issues/26926)
💬 fanquake commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542546467)
Concept ACK - when you get a chance, can you put together a backport PR for `25.x`, so we can see the changes. I think if we want to do that, it'd be preferable to get it into `rc2`.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542546467)
Concept ACK - when you get a chance, can you put together a backport PR for `25.x`, so we can see the changes. I think if we want to do that, it'd be preferable to get it into `rc2`.
💬 fanquake commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542551108)
https://github.com/bitcoin/bitcoin/pull/27125/checks?check_run_id=13380672723
```bash
clang-tidy-16 -p=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/zmq/zmqnotificationinterface.cpp
zmq/zmqnotificationinterface.cpp:48:62: error: std::move of the const variable 'gb' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg,-warnings-as-errors]
return std:
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542551108)
https://github.com/bitcoin/bitcoin/pull/27125/checks?check_run_id=13380672723
```bash
clang-tidy-16 -p=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/zmq/zmqnotificationinterface.cpp
zmq/zmqnotificationinterface.cpp:48:62: error: std::move of the const variable 'gb' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg,-warnings-as-errors]
return std:
...
💬 fanquake commented on pull request "ci: Remove CI_EXEC bloat":
(https://github.com/bitcoin/bitcoin/pull/27616#issuecomment-1542551991)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27616#issuecomment-1542551991)
Concept ACK
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542559776)
Updated 87479b2c3a6e5044baf41343e5287ff4bef0547a -> 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3 ([removeBlockstorageArgs_26](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_27) -> [removeBlockstorageArgs_27](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_27), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_26..removeBlockstorageArgs_27))
* Reverted previous change as suggested by @ryanofsky in [comment](https://github.com/bi
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542559776)
Updated 87479b2c3a6e5044baf41343e5287ff4bef0547a -> 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3 ([removeBlockstorageArgs_26](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_27) -> [removeBlockstorageArgs_27](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_27), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_26..removeBlockstorageArgs_27))
* Reverted previous change as suggested by @ryanofsky in [comment](https://github.com/bi
...
💬 fanquake commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1542571731)
master @ e0a70c5b4f2c691e8d6b507c8ce879f0b0424254. https://cirrus-ci.com/task/5247512164958208:
```bash
node0 2023-05-10T17:29:06.449483Z (mocktime: 2023-05-24T17:29:09Z) [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getrawmempool user=__cookie__
test 2023-05-10T17:29:06.450000Z TestFramework (INFO): Polling buffer...
test 2023-05-10T17:29:06.486000Z TestFramework (INFO): Ensuring mempool:rejected event was handled successfully...
test 2023-05-10T17:29:
...
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1542571731)
master @ e0a70c5b4f2c691e8d6b507c8ce879f0b0424254. https://cirrus-ci.com/task/5247512164958208:
```bash
node0 2023-05-10T17:29:06.449483Z (mocktime: 2023-05-24T17:29:09Z) [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getrawmempool user=__cookie__
test 2023-05-10T17:29:06.450000Z TestFramework (INFO): Polling buffer...
test 2023-05-10T17:29:06.486000Z TestFramework (INFO): Ensuring mempool:rejected event was handled successfully...
test 2023-05-10T17:29:
...
👍 ryanofsky approved a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1421110102)
Code review ACK 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3. Since last ACK just added std::move and fixed commit title. Sorry for the noise!
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1421110102)
Code review ACK 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3. Since last ACK just added std::move and fixed commit title. Sorry for the noise!
⚠️ Spyson opened an issue: "Recovering BTC sent to this Bitcoin address bc1q45puhw8zc5qpgzuuxnefakxckmnldulpgmp0la"
(https://github.com/bitcoin/bitcoin/issues/27619)
(https://github.com/bitcoin/bitcoin/issues/27619)
✅ pinheadmz closed an issue: "Recovering BTC sent to this Bitcoin address bc1q45puhw8zc5qpgzuuxnefakxckmnldulpgmp0la"
(https://github.com/bitcoin/bitcoin/issues/27619)
(https://github.com/bitcoin/bitcoin/issues/27619)
💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190289606)
> Wouldn't it be better to make the insertion conditional on `txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY` ?
Yeah, I think that makes perfect sense, given there is no point in adding something to the filter if we are going to unconditionally serve it anyway.
> Even then this approach seems like it can trigger false positives fairly easily, since the rolling bloom filter capacity is only 3500 txs?
It may indeed. I haven't got my head around how to properly compute what are the odds
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1190289606)
> Wouldn't it be better to make the insertion conditional on `txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY` ?
Yeah, I think that makes perfect sense, given there is no point in adding something to the filter if we are going to unconditionally serve it anyway.
> Even then this approach seems like it can trigger false positives fairly easily, since the rolling bloom filter capacity is only 3500 txs?
It may indeed. I haven't got my head around how to properly compute what are the odds
...
💬 grubles commented on issue "make check errors on big endian OpenBSD 7.2":
(https://github.com/bitcoin/bitcoin/issues/26492#issuecomment-1542674907)
Here's the `feature_addrman.py` output:
```
% ./test/functional/feature_addrman.py --tracerpc
2023-05-10T19:08:15.391000Z TestFramework (INFO): PRNG seed is: 3864180829922889342
2023-05-10T19:08:15.392000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_nfcjzxjq
-1-> getblockcount {}
<-1- [0.001573] 199
...
(https://github.com/bitcoin/bitcoin/issues/26492#issuecomment-1542674907)
Here's the `feature_addrman.py` output:
```
% ./test/functional/feature_addrman.py --tracerpc
2023-05-10T19:08:15.391000Z TestFramework (INFO): PRNG seed is: 3864180829922889342
2023-05-10T19:08:15.392000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_nfcjzxjq
-1-> getblockcount {}
<-1- [0.001573] 199
...
📝 theStack opened a pull request: "test: miner: add coverage for `-blockmintxfee` setting"
(https://github.com/bitcoin/bitcoin/pull/27620)
This PR adds missing test coverage for the `-blockmintxfee` option, which can be used by miners to specify the lowest fee-rate for transactions to be included in blocks. The setting was introduced in PR #9380 (commit daec955fd68bd0da036a5b446b54ffb01108adcd), with the rationale to decouple different minimum fees from `-minrelaytxfee`. According to the PR description it _"should be set by miners to reflect their marginal cost of transmitting extra bytes."_.
On each iteration, the test creates
...
(https://github.com/bitcoin/bitcoin/pull/27620)
This PR adds missing test coverage for the `-blockmintxfee` option, which can be used by miners to specify the lowest fee-rate for transactions to be included in blocks. The setting was introduced in PR #9380 (commit daec955fd68bd0da036a5b446b54ffb01108adcd), with the rationale to decouple different minimum fees from `-minrelaytxfee`. According to the PR description it _"should be set by miners to reflect their marginal cost of transmitting extra bytes."_.
On each iteration, the test creates
...