💬 stickies-v commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1199184833)
Suggested documentation since it's not immediately obvious why this is necessary.
```suggestion
# This allows `test_runner.py` to work from an out-of-source build directory using a symlink,
# hard link or a copy on any platform. See https://github.com/bitcoin/bitcoin/pull/27561.
sys.path.append(tests_dir)
```

👍 stickies-v approved a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1434790272)
ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c modulo improved documentation (left a suggestion).

> The drawback of such an approach is that it creates additional files in the source tree.

You're right, I can't find a way to install the package without it affecting the source tree in one way or another. So the current approach is probably the best approach.

👋 achow101's pull request is ready for review: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467)

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1199199343)
I was thinking that introducing flexibility would enable users to define their preferred threshold for considering fee estimates as stale. As @instagibbs suggested extending the default 12 hours further. what is your view on it?

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1555065798)
Updated 342a0c865d8b4b00a088af7b70b1ee0df1864f5c -> c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 ([pr27561.03](https://github.com/hebasto/bitcoin/commits/pr27561.03) -> [pr27561.04](https://github.com/hebasto/bitcoin/commits/pr27561.04), [diff](https://github.com/hebasto/bitcoin/compare/pr27561.03..pr27561.04)):

- addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1199184833)

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1199252249)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1555065798).

👍 stickies-v approved a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1434932654)
re-ACK https://github.com/bitcoin/bitcoin/commit/c44f3f231988dc05c4c7a8a96bc2e7b1a54da277

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199336080)
> Maybe more ideally we could drop the bool by just having the test call BOOST_CHECK(ShutdownRequested()) and AbortShutdown() after it calls MaybeCompleteSnapshotValidation. This would make the test more realistic and add better coverage, but I didn't check if that worked.

This doesn't work, because calling `StartShutdown` without calling `InitShutdownState` beforehand triggers an `assert(0)`.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1199375548)
> This doesn't work, because calling `StartShutdown` without calling `InitShutdownState` beforehand triggers an `assert(0)`.

I guess then question would be why can't the test framework call `InitShutdownState`? But this suggests that if we remove globals in shutdown code later (I don't know if that's required for the kernel), we should be able to get rid of the `m_shutdown_on_fatal_error` variable pretty easily later too.

💬 mzumsande commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199338711)
Good catch! Is my understanding correct that this race cannot actually happen in practice on master right now, because the indexes are set up (`Init()`) by the init thread, which starts the networking and loadblk threads only later - so I can't see from which other thread `BlockConnected` signals could come from at this stage. However, once index initialization is deferred to the loadblk thread in ca3041984cf3665e27b6783c923ab1c32682500a, I think this race could easily happen.
(this might aff
...

💬 mzumsande commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199268910)
I don't think this is possible to trigger with the way the function is used, but if `upper_block` already has `NO_DATA`, it would return that block, maybe `nullptr` would be more natural? (also applies to `GetFirstStoredBlock`).

Also, the name sounds like a boolean function, maybe something like `CheckBlockDataAvailability()` would be better?

💬 jarolrod commented on issue "crash on macOS 12.6.5":
(https://github.com/bitcoin-core/gui/issues/731#issuecomment-1555287731)
@ddykeman1 would you be able to compile bitcoin core on your own, enabling debug during the configure stage, that would greatly help to identify exactly what is going on.

I'd also suggest sanity checking your setup:
1) Is the data directory existing and available
2) Does the crash occur if you choose a different data directory? (You can run the gui on startup with `-resetguisettings` to be able to choose a new data directory within the gui itself on startup, or pass `-datadir` when running
...

⚠️ bolaubimerah opened an issue: "DEMO SLOT PRAGMATIC > Kumpulan Link Main Gratis Game Terlengkap SLOT DEMO Pragmatic Anti Lag Seperti Asli "
(https://github.com/bitcoin/bitcoin/issues/27704)
# DEMO SLOT PRAGMATIC > Kumpulan Link Main Gratis Game Terlengkap Akun SLOT DEMO Pragmatic Anti Lag Seperti Asli

# **[➡️ DAFTAR GRATIS AKUN DEMO SLOT PRAGMATIC](https://rebrand.ly/daftar-slot-gacor-pay4d)**
# **[➡️ LINK PRAGMATIC SLOT DEMO GRATIS](https://rebrand.ly/daftar-slot-gacor-pay4d)**

[Demo Slot Pragmatic](https://rebrand.ly/daftar-slot-gacor-pay4d) merupakan salah satu dari Demo Slot Online Terpercaya di Indonesia yang mampu memberikan berbagai macam kumpulan link game slot Demo
...

🤔 jarolrod reviewed a pull request: "doc: remove Security section from build-unix.md"
(https://github.com/bitcoin/bitcoin/pull/27688#pullrequestreview-1435213179)
ACK 4bfcbbfd4a9749f7954e37a7447b5f047465bdf8

✅ achow101 closed an issue: "."
(https://github.com/bitcoin/bitcoin/issues/27704)

:lock: achow101 locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/27704)

👍 jarolrod approved a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1435242481)
ACK 78660e72001a2561c7ad3026367a69f65414dbd9

Tested for functionality and sanity checked the user flows between master and this PR.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198931499)
In commit "test: add coverage for db cursor prefix range iteration" (83b4e50291a0bd2c45f370cb18bb479d8f73bc71)

I think you need to add a case like `"\xff\xff"` to test sqlite code without a `key < end_range` condition

Would also consider squashing this commit into earlier commit "wallet: Add GetPrefixCursor to DatabaseBatch" (57f35c90d2ebbc657a781ba3918398720b4319eb) because it's helpful to see test code calling a new function in the same commit that introduces the function.

🤔 ryanofsky reviewed a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1431487191)
This looks great so far! Here is where I am in the review:

- [X] 79075f970802031d42c3808351baa91d5edf9783 wallet: Add GetPrefixCursor to DatabaseBatch (1/18)
- [X] 37a3e64ec84899468aab8c55fd556c93d195e322 walletdb: Consistently clear key and value streams before writing (2/18)
- [X] 39bfce498a7576cd41b2b362ee7688b665472f33 test: add coverage for db cursor prefix range iteration (3/18)
- [X] 3fbbdb2925a2e65b75af9116773e059dd5453afa walletdb: Refactor minversion loading (4/18)
- [X] bc2d888
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1198953172)
In commit "walletdb: Refactor key reading and loading to its own function" (e048fe71038caddb49a5875727a14e05f6edb0d3)

Note: there is a slight change in behavior this commit which could be noted in the commit message.

If this deserialization fails, or if any error conditions are hit below, `wss.nKeys++` will not be incremented anymore. This seems ok, but previously `wss.nKeys++` included the total number of keys found, even ones that could not be loaded.