💬 sipa commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1813041419)
> I haven't used the C++ variant of coroutines either, but my thinking was that since they can theoretically yield execution when waiting for IO (and resume later), this would allow threads to focus on other tasks in the meantime.
That needs async I/O, and is unrelated to coroutines, as far as I understand it. Coroutines just help with keeping track of what to do when the reads come back inside rocksdb.
As long as LevelDB (or whatever database engine we use) internally does not use async I
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1813041419)
> I haven't used the C++ variant of coroutines either, but my thinking was that since they can theoretically yield execution when waiting for IO (and resume later), this would allow threads to focus on other tasks in the meantime.
That needs async I/O, and is unrelated to coroutines, as far as I understand it. Coroutines just help with keeping track of what to do when the reads come back inside rocksdb.
As long as LevelDB (or whatever database engine we use) internally does not use async I
...
💬 edilmedeiros commented on issue "TestFramework: TestShell.reset() will always fail":
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2432635781)
Looks similar, indeed, but I think #30714 did not covered `TestShell().setup().
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2432635781)
Looks similar, indeed, but I think #30714 did not covered `TestShell().setup().
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1813071345)
Oh sorry, I forgot to reply to this. We discussed this in person last week and this is indeed an issue. Happy to take your fix @naumekogs
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1813071345)
Oh sorry, I forgot to reply to this. We discussed this in person last week and this is indeed an issue. Happy to take your fix @naumekogs
💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2432721650)
I'm not in love with the `DEBUG` separation, but I'm fine with it if others are.
ACK 49f73071a8ec4131595090a619d6198a5f8b7c3c
`git range-diff 4f33e9a85c3a8f546deddc2f78cf74bceff30315~4..4f33e9a85c3a8f546deddc2f78cf74bceff30315 aba9ce0eb6e67945f2209a17676f356fe9748
7e6..928538e3782ccbb22f54f4a3b1152b1faba95471`
<details>
<summary>Diff</summary>
```patch
1: 1f2d0ffd56 < -: ---------- move-only: ConstevalFormatString tfm::format to tinyformat.h
2: c937ebf1ce = 1: efa6f3f372 tiny
...
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2432721650)
I'm not in love with the `DEBUG` separation, but I'm fine with it if others are.
ACK 49f73071a8ec4131595090a619d6198a5f8b7c3c
`git range-diff 4f33e9a85c3a8f546deddc2f78cf74bceff30315~4..4f33e9a85c3a8f546deddc2f78cf74bceff30315 aba9ce0eb6e67945f2209a17676f356fe9748
7e6..928538e3782ccbb22f54f4a3b1152b1faba95471`
<details>
<summary>Diff</summary>
```patch
1: 1f2d0ffd56 < -: ---------- move-only: ConstevalFormatString tfm::format to tinyformat.h
2: c937ebf1ce = 1: efa6f3f372 tiny
...
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813106158)
Thanks for pointing out how the logging looks in this PR -- my intent was to preserve the existing behavior in the single transaction replacement case, and not introduce a new line break. (I think the behavior of `LogDebug()` changed recently in #30929, which made what I wrote here no longer do what I expected.)
So we're all on the same page, here's what the logging looks like today (I grabbed this output from the functional tests `feature_rbf.py` and `mempool_package_rbf.py`), in the single
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813106158)
Thanks for pointing out how the logging looks in this PR -- my intent was to preserve the existing behavior in the single transaction replacement case, and not introduce a new line break. (I think the behavior of `LogDebug()` changed recently in #30929, which made what I wrote here no longer do what I expected.)
So we're all on the same page, here's what the logging looks like today (I grabbed this output from the functional tests `feature_rbf.py` and `mempool_package_rbf.py`), in the single
...
💬 Christewart commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432746552)
concept ACK
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432746552)
concept ACK
💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2432766510)
> Would be good to address [#30793 (comment)](https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) as well. Otherwise there will be another follow-up?
I agree that this is a worthwhile update. I'll take a deeper look and see if it might fit nicely into this PR (for less code churn). If not, then it could become its own PR.
> I really think `ORPHAN_MAX_RETENTION_TIME` should be called`ORPHAN_TX_EXPIRE_TIME`
Updated locally. Will push after looking into the above.
>
...
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2432766510)
> Would be good to address [#30793 (comment)](https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274) as well. Otherwise there will be another follow-up?
I agree that this is a worthwhile update. I'll take a deeper look and see if it might fit nicely into this PR (for less code churn). If not, then it could become its own PR.
> I really think `ORPHAN_MAX_RETENTION_TIME` should be called`ORPHAN_TX_EXPIRE_TIME`
Updated locally. Will push after looking into the above.
>
...
💬 tdb3 commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432803849)
> > Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
> > https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
> > Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orpha
...
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432803849)
> > Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
> > https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
> > Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orpha
...
💬 instagibbs commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432809937)
@tdb3 reasonable suggestion, shouldn't take much effort?
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432809937)
@tdb3 reasonable suggestion, shouldn't take much effort?
💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813159728)
> For the package case, given that the txpackages log level is already reporting what transactions are in the package, does it make sense to duplicate that information in the mempool log category as well
No it does not
> An issue that occurred to me with splitting up log messages for what's in a package across multiple lines is that other threads may be logging simultaneously and cause messages to be interleaved, making the resulting log difficult to parse.
Good point
> if we logged
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813159728)
> For the package case, given that the txpackages log level is already reporting what transactions are in the package, does it make sense to duplicate that information in the mempool log category as well
No it does not
> An issue that occurred to me with splitting up log messages for what's in a package across multiple lines is that other threads may be logging simultaneously and cause messages to be interleaved, making the resulting log difficult to parse.
Good point
> if we logged
...
💬 tdb3 commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432815018)
> @tdb3 reasonable suggestion, shouldn't take much effort?
Yeah, I think it would just be moving the logic over to `p2p_orphan_handling`.
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2432815018)
> @tdb3 reasonable suggestion, shouldn't take much effort?
Yeah, I think it would just be moving the logic over to `p2p_orphan_handling`.
💬 achow101 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1813164285)
"The default wallets directory is the 'wallets' folder inside the data directory, unless the `-walletdir` option is specified."
This sentence is a little bit confusing as it suggests `-walletdir` changes the default somehow. I would rephrase this as "The wallets directory is set by the `-walletdir` option and defaults to the 'wallets' folder within the data directory."
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1813164285)
"The default wallets directory is the 'wallets' folder inside the data directory, unless the `-walletdir` option is specified."
This sentence is a little bit confusing as it suggests `-walletdir` changes the default somehow. I would rephrase this as "The wallets directory is set by the `-walletdir` option and defaults to the 'wallets' folder within the data directory."
💬 wonder75 commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2432829746)
I now upgraded to bitcoind 28.0, deleted the blockchain and all indexes. I started a fresh new initial block download. Will report here, if it crashes again. I have a feeling that it could have something to do with starting and stopping the node regulary. My bitcoind only runs for 30 minutes a day to catch up on the blockchain. I start and stop it with cron and systemd.
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2432829746)
I now upgraded to bitcoind 28.0, deleted the blockchain and all indexes. I started a fresh new initial block download. Will report here, if it crashes again. I have a feeling that it could have something to do with starting and stopping the node regulary. My bitcoind only runs for 30 minutes a day to catch up on the blockchain. I start and stop it with cron and systemd.
🤔 stickies-v reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2389456426)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2389456426)
Concept ACK
💬 edilmedeiros commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2432882285)
Just curious if this is still relevant after #30043?
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2432882285)
Just curious if this is still relevant after #30043?
💬 edilmedeiros commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432886068)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2432886068)
Concept ACK
💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2432889367)
When I was initially setting up these systems, I was starting and stopping bitcoind. But since the setup was completed, bitcoind has been running continuously without stopping, and I still saw data corruption.
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2432889367)
When I was initially setting up these systems, I was starting and stopping bitcoind. But since the setup was completed, bitcoind has been running continuously without stopping, and I still saw data corruption.
💬 maflcko commented on issue "How to compile the GUI on opensuse tumbleweed with cmake?":
(https://github.com/bitcoin-core/gui/issues/842#issuecomment-2432924953)
(This worked fine with autotools)
(https://github.com/bitcoin-core/gui/issues/842#issuecomment-2432924953)
(This worked fine with autotools)
💬 maflcko commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2432947751)
Coverage for this target: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/2e7ac47bbc28c2cb/300ddaa72f7e4b9b/fuzz.coverage/index.html
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2432947751)
Coverage for this target: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/2e7ac47bbc28c2cb/300ddaa72f7e4b9b/fuzz.coverage/index.html
💬 edilmedeiros commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2433055899)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2433055899)
Concept ACK