💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2112044166)
Rebased b0594f6cf157e9bd71b2062cdf0aa65936fd2282 -> 46b9ec10dd238c5c1191bcdfcf06d1361cde15f1 ([noGlobalReindex_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_2) -> [noGlobalReindex_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex_2..noGlobalReindex_3))
* Fixed conflict with #30083
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2112044166)
Rebased b0594f6cf157e9bd71b2062cdf0aa65936fd2282 -> 46b9ec10dd238c5c1191bcdfcf06d1361cde15f1 ([noGlobalReindex_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_2) -> [noGlobalReindex_3](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex_2..noGlobalReindex_3))
* Fixed conflict with #30083
💬 maflcko commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1601323608)
Ok, it is a bit stupid to fight this compile warning, but apparently moving it out of the function scope fixes it :shrug:
It only happens with `-stdlib=libc++`.
See https://godbolt.org/z/vjEM7zjzb
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1601323608)
Ok, it is a bit stupid to fight this compile warning, but apparently moving it out of the function scope fixes it :shrug:
It only happens with `-stdlib=libc++`.
See https://godbolt.org/z/vjEM7zjzb
📝 glozow opened a pull request: "refactor: TxDownloadManager"
(https://github.com/bitcoin/bitcoin/pull/30110)
Part of #27463.
Draft while #30000 is not merged, and because I am still smoothing out the interface, making sure the commits are clean, and writing more tests.
This modularizes transaction download logic into a `TxDownloadManager`. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1]
All of the changes here should result in no behavior change.
There are several benefits to this interface, such as:
- Unit test coverage a
...
(https://github.com/bitcoin/bitcoin/pull/30110)
Part of #27463.
Draft while #30000 is not merged, and because I am still smoothing out the interface, making sure the commits are clean, and writing more tests.
This modularizes transaction download logic into a `TxDownloadManager`. Transaction download logic refers to the process of deciding what transactions to request, download, and validate.[1]
All of the changes here should result in no behavior change.
There are several benefits to this interface, such as:
- Unit test coverage a
...
💬 laanwj commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2112065633)
Concept ACK as i said here: https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2091354481
> I'd rather thread_local be safe to use (in all circumstance) on a platform, or we just not use it.
Sure, that'd be ideal, but handling destructors with TLS is *very hard* and easy to get wrong. It's not something we want to rely on.
Ideally we'd get rid of thread-local usage completely. But we've considered alternative solutions, and tracking some data until a thread disappears is hard
...
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2112065633)
Concept ACK as i said here: https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2091354481
> I'd rather thread_local be safe to use (in all circumstance) on a platform, or we just not use it.
Sure, that'd be ideal, but handling destructors with TLS is *very hard* and easy to get wrong. It's not something we want to rely on.
Ideally we'd get rid of thread-local usage completely. But we've considered alternative solutions, and tracking some data until a thread disappears is hard
...
💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1601351165)
You're referring to the 1p1c logic, which isn't executed here (notice that all of the feerates are "normal" feerates).
Here, the orphans are reconsidered with `ProcessOrphanTx`, which is called at the top of `ProcessMessages`.
https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L5355-L5369
`ProcessMessages` is called for each peer in a random order
https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net.cp
...
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1601351165)
You're referring to the 1p1c logic, which isn't executed here (notice that all of the feerates are "normal" feerates).
Here, the orphans are reconsidered with `ProcessOrphanTx`, which is called at the top of `ProcessMessages`.
https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L5355-L5369
`ProcessMessages` is called for each peer in a random order
https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net.cp
...
💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1601354578)
It is rejected. The acceptance of the parent triggers the node to reconsider all children in orphanage (grep `AddChildrenToWorkset`. If the failure isn't `TX_MISSING_INPUTS`, it is removed from orphanage.
https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3402-L3414
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1601354578)
It is rejected. The acceptance of the parent triggers the node to reconsider all children in orphanage (grep `AddChildrenToWorkset`. If the failure isn't `TX_MISSING_INPUTS`, it is removed from orphanage.
https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3402-L3414
🤔 willcl-ark reviewed a pull request: "contrib: use ENV flags in get_arch"
(https://github.com/bitcoin/bitcoin/pull/30074#pullrequestreview-2057646945)
post-merge ACK b59a027d957a4cffd225a681e6c85f9ae7fd77f3
My hashes match, but don't include the `output/*/SHA256SUMS.part` output lines:
```
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
96c131400a038434f5338926d72c62d7fa260daf3dde9966c7d7880a182b0b6e bitcoin-b59a027d957a-arm-linux-gnueabihf-debug.tar.gz
...
(https://github.com/bitcoin/bitcoin/pull/30074#pullrequestreview-2057646945)
post-merge ACK b59a027d957a4cffd225a681e6c85f9ae7fd77f3
My hashes match, but don't include the `output/*/SHA256SUMS.part` output lines:
```
15310e3aff2a9ef71bad06315fcd425e56611d5bed7f6c394f3fe2248140e17b bitcoin-b59a027d957a-aarch64-linux-gnu-debug.tar.gz
8e60146d39a47e9c332e3b842e239f1ca7773ca2e6788876155377f4568ab1fa bitcoin-b59a027d957a-aarch64-linux-gnu.tar.gz
96c131400a038434f5338926d72c62d7fa260daf3dde9966c7d7880a182b0b6e bitcoin-b59a027d957a-arm-linux-gnueabihf-debug.tar.gz
...
👍 TheCharlatan approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2057650383)
Re-ACK e41667b720372dae8438ea86e9819027e62b54e0
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2057650383)
Re-ACK e41667b720372dae8438ea86e9819027e62b54e0
👍 stickies-v approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2057592650)
ACK dacdb7962c5fef8db26f6fa31facb606165d1d1e
nit: some IWYU fixes, mostly because of the newly introduced `bilingual_str` and `Assert` usage:
<details>
<summary>git diff on dacdb7962c</summary>
```diff
diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
index a52c566076..3cc28a2b14 100644
--- a/src/test/fuzz/mini_miner.cpp
+++ b/src/test/fuzz/mini_miner.cpp
@@ -12,6 +12,8 @@
#include <primitives/transaction.h>
#include <random.h>
#include <txmempool.h>
...
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2057592650)
ACK dacdb7962c5fef8db26f6fa31facb606165d1d1e
nit: some IWYU fixes, mostly because of the newly introduced `bilingual_str` and `Assert` usage:
<details>
<summary>git diff on dacdb7962c</summary>
```diff
diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
index a52c566076..3cc28a2b14 100644
--- a/src/test/fuzz/mini_miner.cpp
+++ b/src/test/fuzz/mini_miner.cpp
@@ -12,6 +12,8 @@
#include <primitives/transaction.h>
#include <random.h>
#include <txmempool.h>
...
💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601391240)
nit: perhaps move semantics would make sense here?
```suggestion
node.mempool = std::make_unique<CTxMemPool>(std::move(mempool_opts), mempool_error);
if (!mempool_error.empty()) {
return InitError(mempool_error);
}
LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), node.mempool->m_opts.max_size_bytes * (1.0 / 1024 / 1024));
```
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601391240)
nit: perhaps move semantics would make sense here?
```suggestion
node.mempool = std::make_unique<CTxMemPool>(std::move(mempool_opts), mempool_error);
if (!mempool_error.empty()) {
return InitError(mempool_error);
}
LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), node.mempool->m_opts.max_size_bytes * (1.0 / 1024 / 1024));
```
💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601457942)
nit: this approach has the potential risk that if additional error handling is added to the CTxMemPool ctor, the fuzzer would silently ignore it if this test is not also updated. Could avoid that by specifically silencing only this error?
<details>
<summary>git diff on dacdb7962c</summary>
```diff
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index eefa566d31..1a95ff4ab6 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@
...
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601457942)
nit: this approach has the potential risk that if additional error handling is added to the CTxMemPool ctor, the fuzzer would silently ignore it if this test is not also updated. Could avoid that by specifically silencing only this error?
<details>
<summary>git diff on dacdb7962c</summary>
```diff
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index eefa566d31..1a95ff4ab6 100644
--- a/src/test/fuzz/package_eval.cpp
+++ b/src/test/fuzz/package_eval.cpp
@@
...
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601479946)
Not sure about this if it gets moved again when the for loop iterates.
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601479946)
Not sure about this if it gets moved again when the for loop iterates.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2112341321)
Thanks!
On Windows (Vista and higher, which is all we care about) getting the default gateway is straightforward, there's `GetBestRoute2` that does all the work. It's part of netioapi which we already need for interface enumeration. MacOS's sysctl is a bit uglier.
> If PCP servers announced themselves via DNS-Based Service Discovery [rfc6763](https://www.rfc-editor.org/rfc/rfc6763.html) that would also make things easier.
i don't think it really would, we'd need some special library for
...
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2112341321)
Thanks!
On Windows (Vista and higher, which is all we care about) getting the default gateway is straightforward, there's `GetBestRoute2` that does all the work. It's part of netioapi which we already need for interface enumeration. MacOS's sysctl is a bit uglier.
> If PCP servers announced themselves via DNS-Based Service Discovery [rfc6763](https://www.rfc-editor.org/rfc/rfc6763.html) that would also make things easier.
i don't think it really would, we'd need some special library for
...
💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2112349111)
rebased due to merge conflict
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2112349111)
rebased due to merge conflict
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2112366413)
rebased due to conflict
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2112366413)
rebased due to conflict
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112374433)
Re https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2057592650
> nit: some IWYU fixes, mostly because of the newly introduced bilingual_str and Assert usage:
A bunch of these were broken before this PR, so I wasn't sure if they make sense to repair here. For test files I stopped asking for repairing them when reviewing other PRs, it just feels too Sisyphean if they are not consistently checked by other reviewers anyway. I wish we'd start enforcing this properly though.
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112374433)
Re https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2057592650
> nit: some IWYU fixes, mostly because of the newly introduced bilingual_str and Assert usage:
A bunch of these were broken before this PR, so I wasn't sure if they make sense to repair here. For test files I stopped asking for repairing them when reviewing other PRs, it just feels too Sisyphean if they are not consistently checked by other reviewers anyway. I wish we'd start enforcing this properly though.
💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601574911)
nit: `JSONErrorReply` -> `JSONRPCErrorReply`, although it could be argued that it actually does write a JSON object in the response.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601574911)
nit: `JSONErrorReply` -> `JSONRPCErrorReply`, although it could be argued that it actually does write a JSON object in the response.
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112426141)
Thank you for the review @stickies-v,
Updated dacdb7962c5fef8db26f6fa31facb606165d1d1e -> 27af3910e5fa9915d4913b82443db216d1b1d61b ([mempoolArgs_7](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_7) -> [mempoolArgs_8](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_7..mempoolArgs_8))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601457942), checking s
...
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112426141)
Thank you for the review @stickies-v,
Updated dacdb7962c5fef8db26f6fa31facb606165d1d1e -> 27af3910e5fa9915d4913b82443db216d1b1d61b ([mempoolArgs_7](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_7) -> [mempoolArgs_8](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_8), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_7..mempoolArgs_8))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601457942), checking s
...
💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2112430969)
reACK 186a00e644 via range-diff
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2112430969)
reACK 186a00e644 via range-diff
💬 glozow commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#issuecomment-2112437062)
reACK 9365baa489
(https://github.com/bitcoin/bitcoin/pull/30066#issuecomment-2112437062)
reACK 9365baa489