🤔 pablomartin4btc reviewed a pull request: "test: Check already deactivated network stays suspended after dumptxoutset"
(https://github.com/bitcoin/bitcoin/pull/30892#pullrequestreview-2301535327)
ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa

💬 pablomartin4btc commented on pull request "test: Check already deactivated network stays suspended after dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/30892#discussion_r1757670575)
nit:
```suggestion
self.log.info(f"Test that dumptxoutset with rollback type fails given an invalid height")
assert_raises_rpc_error(
```

💬 fjahr commented on pull request "test: Check already deactivated network stays suspended after dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/30892#discussion_r1757681797)
Hm, I don't think it makes sense to add that. First, the actual error here is not the point of this test. This test is about the network activity and even if it covers something else I would like that to be structured differently then and have the logs be in `run_test`. Where it is now it would also be printed twice. Second, the content isn't right: The reason this fails is not the rollback height, it's because the rev file can not be read, so the rolling back action failed as in, it could not r
...

💬 Roasbeef commented on pull request "rename TransactionError:ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2347371722)
Does `bitcoind` consider this to be a breaking change? The new error message may be "more accurate", but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.

Alternatively, the error constant could be renamed, with the error text/string left in place.

💬 pablomartin4btc commented on pull request "test: Check already deactivated network stays suspended after dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/30892#discussion_r1757707660)
Yes, that's correct. Thanks! I got confused by the error message, block height was actually valid (`COINBASE_MATURITY = 100`), then we are missing testing the rollback type given an invalid height (?).

🤔 pablomartin4btc reviewed a pull request: "gui: fix crash when closing wallet"
(https://github.com/bitcoin-core/gui/pull/835#pullrequestreview-2301586812)
ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e

Went thru the code and as commented in the new `WalletController::removeWallet` function that's now been called instead of `removeAndDeleteWallet`, it will end up calling later `removeAndDeleteWallet` (within the connection to `unload` signal in `WalletController::getOrCreateWallet` - `unload` triggered by `wallet().remove()`) which will emmit `walletRemoved` connected to `BitcoinGUI::removeWallet` (this for example will remove the wallet from the
...

💬 vostrnad commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2347429510)
I'm happy to report that a quick benchmark of 4cfff4e58c6d806e4bc5a12386f84ff207c83419 cherry-picked onto v28.0rc1 shows that the slowdown is almost or entirely gone. I'll do a more thorough benchmark later once the code is ACKed.

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757737177)
I don't have a strong view, but I'm trying to think about what a mining pool might want the behavior to be. One guess is that some miners may want to have a way to prioritise normal-looking transactions but not inadvertently prioritise something that would create a dust output and spam the UTXO set with garbage that will never be spent.

If that is the case, then we could try to build the heavier machinery you're talking about: perhaps change the behavior of `prioritisetransaction` to produce
...

✅ fabioBaraDev closed an issue: "No such file or directory: bitcoind Error"
(https://github.com/bitcoin/bitcoin/issues/30891)

💬 fabioBaraDev commented on issue "No such file or directory: bitcoind Error":
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347671266)
I found out...
I use sentinel one as antivirus,

💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2347927495)
> I ran benchmarks to evaluate the impact of removing the limit on IBD performance. ... I synced up 3 times to block height 600,000...

Thanks for this benchmark. The current block height is 861,090 and the size of the utxo set has more than doubled since 600,000 so this is why you do not see performance improvements from dbcache > 10 GB.

> Based on this data I think removing the limit may not be substantiated...

Try syncing to 860,000 there should be performance benefit up to 30 GiB dbc
...

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758096576)
Only commenting because this confused me:

This makes sense since the third possibility here is that `origin == SEEK_END`, in which case the only way for `fseek` to have returned zero is if `offset` is a negative number offset from the end of the file, and `m_position = length_of_file + negative_offset`

But, in order to get the length of the fil you need to `fseek(m_file, 0, SEEK_END)` and `ftell(m_file)`.

So, just `ftell` here is the best way to get the offset.

🤔 BenWestgate reviewed a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2302029807)
crACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807.

Setting `dbcache` below available RAM performs better and swaps less than setting it below total RAM, which may even crash the machine or OOM kill other applications.

I feel the tool tip and argument texts should say "available RAM" but it's not a blocker for me.

💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1758106872)
Nit: "Make sure you have enough available RAM." seems more correct.

💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#discussion_r1758098327)
nit: This should say "RAM available" rather than "RAM".

On a 64 GB system without swap: it will crash if you set `dbcache=32000` but only have only have 20 GB available.

On a 64 GB system with enough swap: it will perform MUCH worse and prematurely wear out the flash storage, if you set `dbcache=32000` but only have only have 20 GB available RAM.

(I've destroyed new USB sticks overnight, putting a swapfile on them and doing IBD with `dbcache` set a little too high.)

The only reason
...

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758143719)
Outside of the scope of this PR: I feel like this should probably handle a failed `fread` explicitly, even though the subspan call below means XOR won't do anything bad here, and when this returns to `AutoFile::read()` or `BufferedFile::fill()` (it's only two callers) the error gets handled. It seems kind of footgun-like to let this potentially dangerous file pointer get passed around and I'm not sure it's worth `BufferedFile` being able to print it's own name when throwing an exception.

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758185734)
clang-tidy seems [upset](https://github.com/bitcoin/bitcoin/pull/30884/checks?check_run_id=30079004954) about setting `m_position{0}` here instead of using C++11 "Default member initialization" in the class declaration (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)

👍 jarolrod approved a pull request: "gui: fix crash when closing wallet"
(https://github.com/bitcoin-core/gui/pull/835#pullrequestreview-2302134082)
ACK a965f2bc07a3588f8c2b8d6a542961562e3f5d0e

This accurately diagnoses and fixes the referenced issue. I've thoroughly tested this on Linux, macOS, and Windows; and both confirm the existence of the bug and that this fixes it. I've also sanity tested other related actions in the GUI in different scenarios, and also sanity tested other non-wallet actions in the GUI for crashes.

Thanks for the quick fix :)

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1758190241)
nit: Handle ftell error

```suggestion
if (pos < 0) {
throw std::ios_base::failure("AutoFile::seek: ftell failed");
}
m_position = pos;
```

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2348023783)
@ajtowns

> If this is just something a peer can opt-in to, I don't think it mitigates any DoS behaviours: spammers/attackers will just use the old version.

Yes, indeed this is a quite obvious point about a node service bit. It was raised during the mempoolfullrbf discussion years ago (see #25600), that effectively node service bit network mechanism cannot be effectively checked for, without actually connecting to and triggering the logic. About the present case, this observation has been
...